Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganisation of experiment #515

Open
MarcusZuber opened this issue Jan 31, 2023 · 3 comments
Open

Reorganisation of experiment #515

MarcusZuber opened this issue Jan 31, 2023 · 3 comments
Assignees
Labels

Comments

@MarcusZuber
Copy link
Member

Currently all abstract experiments (Radiography, different Tomography implementations, grating interferometry) are in concert.experiments.imaging and the impementations for X-ray setups and beamlines in concert.experiments.synchrotron and concert.experiments.xraytube. These modules are gelling longer (and more chaotic).

I would propose to split everything in smaller modules (and skip the "imaging" since all experiments are for imaging) like

concert.experiments.abstract.radiography
concert.experiments.abstract.tomography
concert.experiments.abstract.grating_inteferometry

concert.experiments.synchrotron.radiography
concert.experiments.synchrotron.tomography
concert.experiments.synchrotron.grating_inteferometry

etc.

This will brake some imports for the users, but could help to make this code more structured (and a user does not have to find the important part of the code in a 1000+ lines long file).

@MarcusZuber MarcusZuber self-assigned this Jan 31, 2023
@tfarago
Copy link
Contributor

tfarago commented Jan 31, 2023

Or do we need that many classes? Basically for every special combination of an experiment methodand specific harswarewe have a separate class. Could we have fever classes with some configuration instead of inheritance?

@MarcusZuber
Copy link
Member Author

In general I think the configuration could be a good implementation.

But to keep parameters in the experiments the conf. instance has to register the parameters to the exp class (on instance and not class level, see my PR).

Also, to make this work with the "users" we have to write factory functions for all possible combinations, which will then be almost the same as the current classes with the different mixins.

(Also I'm happy that a few people using this started to get some idea of inheritance and I don't want to introduce the next thing to them).

PS: Due to your typo I was thinking the whole evening what pattern I was missing...

@tfarago
Copy link
Contributor

tfarago commented Feb 1, 2023

oops, sorry. Also, please keep in mind #481, it would be good if all "big" changes came after that's been merged. Which reminds me that I should get back to that one...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants