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

Combine raw & proc in open_run() by default #569

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

takluyver
Copy link
Member

This adds a new data='default' option (which becomes the default) to combine raw & proc data, preferring raw where names clash. For new data, this means you can access corrected detector data with the new source names (e.g. */CORR/*), alongside raw data with names like */DET/*. For older data, there will be no difference from data='raw'.

  • This & Allow selecting raw/proc data in components layer #468 together aren't 100% backwards compatible. Code that calls open_run() without passing data= and then uses a detector component like AGIPD1M() previously got raw data, but would now get corrected data when reading runs with the new source names. But hopefully this isn't common, and it's easily fixed.
  • I'm not completely sure about when to error or warn on missing folders. At present, this errors if neither raw or proc run folders exist, and warns if only proc exists (somehow). It won't complain about a missing proc folder if run exists.

A large part of the diff is just moving some existing tests to a new file, so it may be easier to review by commit.

@takluyver takluyver added the enhancement New feature or request label Nov 1, 2024
Copy link
Contributor

@philsmt philsmt left a comment

Choose a reason for hiding this comment

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

I gave the tests only a brief look, as I suspect the non-default parts are copied verbatim.

The main logic looks good. LGTM, apart from some minor code flow recommendations.

extra_data/reader.py Outdated Show resolved Hide resolved
if origin not in absence_ok:
if base_dc is None:
raise
warn(f'No data available for this run at origin {origin}')
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably go for a continue in the except branch, and move the entire else branch to after the complete try-except to avoid a level of indentation for so much code. I realize this has been like this before (written by me 🙃), but the logic seems complicated enough now that it seems useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, done

@takluyver
Copy link
Member Author

This now also fixes a bug @JamesWrigley noticed, where specifying _use_voview=False was ignored when combining multiple origins.

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

Successfully merging this pull request may close these issues.

2 participants