-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add gui for batch reduction #144
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
This is ready for review 👍 |
src/ess/reflectometry/gui.py
Outdated
|
||
|
||
class NexusExplorer: | ||
def __init__(self, runs_table, run_to_filepath): |
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.
Can you briefly type hint these? If it's sth simple like a dictionary or tuple...
widgets.VBox( | ||
[ | ||
widgets.Label("Auto Reduction Table"), | ||
self.reduction_table, | ||
], | ||
layout={'margin': '10px 0'}, |
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.
(Code-style) It looks like this kind of VBox
with a label and a sub widget appears multiple times.
Maybe we can write a helper function and reduce some effort...?
But on the other hand it's not so complicated and probably easier to modify individual Boxes in this way so it's up to you.
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.
For now I'll keep it like it is
with h5py.File(path) as f: | ||
return { | ||
"Sample": f['entry1']['sample']['name'][()][0].decode('utf8'), | ||
"Run": path[-8:-4], |
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.
Run number is also available in the file as far as I understand.
Looks like it's experiment_identifier
. so maybe...
f['entry']['experiment_identifier'][()][0].decode('utf8')
. is better...?
Also because ECDC/DST will change how the files are named soon and users might just copy the files and name it whatever they want.
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.
In this case these are Amor files, and they don't have the run number in the files :/
This method will have to be re-implemented on a per-instrument basis.
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.
Good to know... that's a bit unfortunate...!
except StopIteration: | ||
# No results were found for the selected row | ||
# It hasn't been computed yet, so compute it and try again. | ||
self.run_workflow() |
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.
Won't it fall into an infinite loop if run_workflow
fails before it sets the result?
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.
Good catch!
self.custom_reduction_table.data = df | ||
self.set_table_colors(self.custom_reduction_table) | ||
|
||
def display_results(self): |
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.
This function does quite a lot of things. Can you add a brief docstring what it does?
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've added a brief description to the method in the superclass 👍
Rebase of #138