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

Refactor parameter checks #251

Merged
merged 28 commits into from
Apr 23, 2022
Merged

Refactor parameter checks #251

merged 28 commits into from
Apr 23, 2022

Conversation

carnisj
Copy link
Owner

@carnisj carnisj commented Apr 18, 2022

Before this modification, parameters check and configuration were all over the place in bcdi_strain.py and bcdi_preprocessing_bcdi.py. Now everything is managed by a single class.

Partially related to #146

  • implement the class bcdi.utils.parameters.ConfigChecker and the child classes bcdi.utils.parameters.PostprocessingChecker and bcdi.utils.parameters.PreprocessingChecker to perform the script-dependent configuration of parameters.
  • the parameter scan in bcdi_strain.py was renamed scans to match preprocessing terminology
  • the parameter sdd was renamed detector_distance to match PyNX terminology
  • it is now possible to perform a strain analysis of several scans y providing the scan numbers as a list, and if relevant data_dir, save_dir, specfile_name, template_imagefile and reconstruction_file as lists of strings
  • parameters from the config are accessed directly from the config dictionary: this will help when later refactoring the runners (splitting the scripts into meaningful small functions)

Type of change

  • Breaking change (will cause existing functionality to not work as expected)

How has this been tested?

Example dataset with a series of 2 scans (post & preprocessing)

Checklist:

  • I have made corresponding changes to the documentation
  • I have added corresponding unit tests
  • I have run doit and all tasks have passed

@carnisj carnisj marked this pull request as draft April 18, 2022 19:10
@carnisj
Copy link
Owner Author

carnisj commented Apr 18, 2022

I am still working on adding unit tests.

The interesting parts are the classes ConfigChecker, PostprocessingChecker and PreprocessingChecker and the way they are instantiated in preprocessing_runner.py (line 188) and postprocessing_runner.py (line 44).

The rest is just (a lot of) cleaning up.

Copy link
Collaborator

@DSimonne DSimonne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I saw the changes are consistent, looks nice !

@clatlan
Copy link
Collaborator

clatlan commented Apr 19, 2022

Thanks, it seems great.
By the way, I have just noticed that 'None' is not understood by yaml as python does. I have to write 'null' instead.

@carnisj
Copy link
Owner Author

carnisj commented Apr 19, 2022

Thanks, it seems great. By the way, I have just noticed that 'None' is not understood by yaml as python does. I have to write 'null' instead.

None will be loaded as the string 'None', then you have to convert it to Python None when validating the parameters (this is done in parameters.valid_params)

@clatlan
Copy link
Collaborator

clatlan commented Apr 19, 2022

Yes. Then it means the run() functions of preprocess_runner and postprocess_runner implicitly need parameters that have already been loaded by bcdi. I parse my own dictionaries to these runners. Anyway, problem solved with 'null' (directly converted to None when read by yaml python loader).

@carnisj carnisj marked this pull request as ready for review April 23, 2022 19:38
@carnisj carnisj merged commit 89bc6c7 into main Apr 23, 2022
@carnisj carnisj deleted the scans branch April 23, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants