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

Add support to open run data from any number of data locations #298

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

philsmt
Copy link
Contributor

@philsmt philsmt commented Mar 22, 2022

Somewhat coincidental to our internal discussion on differentiating data from raw and proc, I already planned to extend the data argument of open_run to allow specifying multiple and arbitrary locations relative to the proposal root. The main use case is the ONC offline corrections, which by default output to <proposal>/scratch/cal. With this PR, you could open this like:

run = open_run(proposal=123, run=456, data=['raw', 'scratch/cal'])

The previous case data='all' is implemented as an alias for ['raw', 'proc'], and it preserves the overwriting rules we established for it.

@takluyver
Copy link
Member

Thanks! This looks good, but I might be inclined to hold it until we can look data up by origin in the case of clashing names as well (something like run.get('MID_DET_AGIPD1M-1/DET/0CH0:xtdf', 'image.data', data='proc')). That's going to require a more involved change, and if we're not careful we might accidentally make it harder to deal with that.

@philsmt
Copy link
Contributor Author

philsmt commented Dec 20, 2023

Do you think we're still going for some layer implementation or we can continue here?

@takluyver takluyver force-pushed the feat/open-multiple-origins branch from 450c0eb to 0a5ba6a Compare March 5, 2024 15:05
@takluyver
Copy link
Member

I've rebased this onto current master. Now that loading aliases no longer prints a message (#478), I've also simplified the logic a little bit to just load the aliases again for each origin.

@philsmt would you have time to write a test for this?

@takluyver takluyver added this to the 1.17 milestone Mar 5, 2024
@takluyver takluyver added the enhancement New feature or request label Mar 5, 2024
@philsmt philsmt force-pushed the feat/open-multiple-origins branch from 0a5ba6a to 018538e Compare March 6, 2024 17:32
@philsmt
Copy link
Contributor Author

philsmt commented Mar 6, 2024

Thanks, I did! The diff looks weird, but essentially I moved the bits testing data='all' from test_open_run into test_open_run_multiple parametrizing all and ['raw', 'proc'].

@takluyver
Copy link
Member

Thanks, LGTM

@philsmt
Copy link
Contributor Author

philsmt commented Mar 8, 2024

Thanks for review, probably my MR with the longest review time. Could've waited the two weeks to make it 2 years 😁

@philsmt philsmt merged commit c472b91 into master Mar 8, 2024
10 checks passed
@tmichela tmichela deleted the feat/open-multiple-origins branch October 1, 2024 10:21
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