-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor run temp dependant function #876
Conversation
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 refactor looks good. I just had a few suggestions and questions. In particular, I think it would be worth checking with a user that the sample temperature doesn't need to be cached before removing this functionality
norm = Normalize(vmin=intensity_start, vmax=intensity_end) | ||
slice = compute_slice(selected_ws, x_axis, y_axis, norm_to_one) | ||
self._cache_slice(slice, colourmap, norm, sample_temp, x_axis, y_axis) | ||
self._cache_slice(slice, colourmap, norm, None, x_axis, y_axis) |
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.
Would it be possible to check with the users that caching the sample temperature is no longer wanted before we remove it?
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 feature was accidently removed a couple of months ago, so I think we're safe to remove. I'll raise an issue to investigate whether this should be reimplemented. At this point it's more effort to fix than to remove.
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, I think it's fine to remove it. In practice different slices often have different temperatures so the caching is not always useful.
Is _cache_slice
referenced elsewhere where it needs the temperature? If not, maybe it can be refactored to remove this argument, instead of passing None
?
Ideally what we could have in future is to cache the name of the log variable which holds the temperature and use that to determine the temperature of the slice. However (1) this was never implemented, and more importantly (2) the .nxspe
file format we often use discards all the log information :( the reduced/processed .nxs
files retains them but it's hard to get users to change to using it...
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 is not used elsewhere. I think I will leave it as is now, and implement the feature as you outline shortly #877.
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.
Thanks for the changes, happy to approve
Description of work:
Refactors a large function that was getting hard to work with.
Also, I've removed the caching of sample log fields on the
slice_plotter_presenter
. This used to mean that you only had to select a sample log for temperature once, then it would be used for all other subsequent slices. I accidently bypassed this functionality a while ago when adding intensity change for cuts, taking the view that the temperature should be specified for every new dataset introduced.If users take issue with this, I can reimplement.
To test: