-
Notifications
You must be signed in to change notification settings - Fork 23
apply data_range to all columns, including T, dT, L, dL #331
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
base: master
Are you sure you want to change the base?
Conversation
This is affecting a currently-running experiment at ORNL - so would like to get merged soon! |
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.
Ick. Can we instead force the arity of T,L,dT,dL to match R?
You mean before we apply the slice? Is there a compact way to do that? It's tricky because they can be None, too. I didn't check what would happen if they were a scalar - if the load would still work. |
An alternative fix is to use the following in np.asarray(v)[index] if hasattr(v, '__getitem__') else v And maybe rename the function to The existing code was already applying the index, but only if the item was a list. The ORSO reader is setting the values to numpy arrays, so the |
…ed, and allow specifying uncertainty for a column in a header field
… while also using data_range kwarg on load4
(i for i, c in enumerate(columns) if getattr(c, "physical_quantity", None) == orso_name), | ||
None, | ||
) | ||
resolution_index = None |
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.
get_key() does not need to be defined in the for loop. It picks up the local scope regardless of where things are defined, so long as it is defined before it is used.
# third column is always uncertainty on reflectivity | ||
dR_col = columns[2] | ||
if dR_col.value_is == "FWHM": | ||
data[:, 2] = FWHM2sigma(data[:, 2]) |
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 modifies the entry in place as returned by the orso/nexus reader. That's probably okay, but since you are copying the data anyway on line 130, you may as well move the copy to line 62 so that you are operating on your own copy of the data. You might also force the data type to be float64 when you create the copy.
I'm a little concerned that ΔR is potentially being scaled by FWHM2sigma. Is there any risk that the ΔR column is mislabeled as FWHM, e.g., because it is defaulting to FWHM when the value isn't specified? This would be happening in the orso/nexus readers.
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.
Yes, there's a risk that the dR is not properly tagged as sigma
or FWHM
in the ORSO file. I don't really know how to mitigate that, since each "error" column or ErrorValue in the ORSO format independently describes whether it is sigma
or FWHM
. With that level of complexity, a single FHWM
boolean flag like what we have in load4 is not helpful.
At least, in the implementation here, if the ORSO file is correctly described, it will correctly load, even if sigma
and FWHM
types are both present.
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 guess we could separate the parse step from the step that fills in the Probe classes, and allow users to edit parts of the ORSO classes before loading them...
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.
If the column is mislabeled in the stored file that's on the facility. The user can correct this manually after loading the file. My concern was whether the orso or nexus loaders default the ΔR column to FWHM if it is not otherwise labelled in the file.
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.
No, the value_is
attribute does not default to "FWHM"
- it actually defaults to None
, which I guess we're interpreting to mean sigma
here. In the ORSO code, it is indicated that not specifying FWHM
implies that it is sigma
.
Fixes an issue where if a dataset with more than 4 columns is loaded (including T, dT, L or dL) and the
data_range
flag is used, then the Q, R, dR, dQ columns are trimmed to the data range, but the additional columns are not.