-
Notifications
You must be signed in to change notification settings - Fork 3
docs: add more example files and change data fetching functions #152
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
Conversation
src/ess/amor/data.py
Outdated
| def amor_run(number: int | str, year: int | str = 2024) -> Filename[SampleRun]: | ||
| return Filename[SampleRun](_pooch.fetch(f"amor{year}n{int(number):06d}.hdf")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the year needed? Aren't run number unique? At least that is what I have seen from other facilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just simpler to implement that way. We could either rename the files to exclude the year, or list the files an find the one with matching run number. Do you think it's better to go with one of those approaches?
(That said, I don't know if the run numbers are unique or not. I've never seen duplicates, so I don't doubt they're unique if that is also your experience.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if the year is required these functions are essentially unusable without looking into the source code, right? Unless you did the experiment yourself and remember what year it was in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the year argument
| "# The sample rotation value in the file is slightly off, so we set it manually\n", | ||
| "workflow[SampleRotation[ReferenceRun]] = sc.scalar(0.65, unit='deg')\n", | ||
| "workflow[Filename[ReferenceRun]] = amor.data.amor_reference_run()\n", | ||
| "workflow[Filename[ReferenceRun]] = amor.data.amor_run(614)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above you passed runs as string, but an int here? Should it all be ints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both work but I agree it's better to be consistent.
Adds more example files to be used in example notebook Nico is working on, slightly changes the interface to be able to access files from different years.