Ticket #8906 (closed: fixed)
Improve handling monitors in SANS
Reported by: | Gesner Passos | Owned by: | Peter Parker |
---|---|---|---|
Priority: | critical | Milestone: | Release 3.1.1 |
Component: | SANS | Keywords: | PatchCandidate |
Cc: | anders.markvardsen@… | Blocked By: | |
Blocking: | #8921, #8953 | Tester: | Gesner Passos |
Description
The user complained that the loading more recent files does not show the monitors. In their operation, they use the monitors to check if the data was 'nice'.
Besides, internally, we are handling monitors poorly, loading it more than once, and not keeping track of them.
I suggest the following:
1 - Add the flag LoadMonitors=True to the loader (LoadRun class). 1b - Ideally, all the monitors will have a descriptive name and should be grouped together inside a group name Monitors. (I think we are creating a 'Monitors' workspace in the process, it must be checked). 2 - Add to the class Sample a method like this:
- def get_monitors(self): return monitors (workspace)
This method will be responsible for handling the monitors when they were loaded separately (event workspaces) and when they were loaded together (histogram workspace). This will be also a 'const' method in the sense, that it will assume clients of this method will not change the monitors workspaces, if they need so, they will clone it beforehand.
3 - Remove the method get_monitor from the reducer, and substitute it where it is called for the method in the sample through get_sample().get_monitors()
- The signature is slightly different, but it is just one algorithm that is executed that the clients could execute: ExtractSingleSpectrum. Or, if you want, you can add the extra option to the Sample class.
4 - Update SliceEvent to get the monitor from the Sample class and not 'guessing' it from the workspace.
This last point will solve an issue also. If you try to load and reduce the simulated data created by Richard (babylon Freddies folder) SANS2d90024183.nxs, it will break on SliceEvent due to an error about the assumption to guess the file path.
Change History
comment:2 Changed 7 years ago by Gesner Passos
- Blocking 8953 added
(In #8953) This ticket depends on the solution proposed in #8906. The problem is that in SANSRunWindow:setGeometryDetails there is a point it tries to get the monitor, if it fails it silently give up doing the job. Hence, it will depend on the solution proposed there to be able to handle this issue.
comment:4 Changed 7 years ago by Gesner Passos
Comment from Richard related to this ticket:
Note that loaded nxs workspaces only have event mode data (thus may only appear to have one time channel), not monitor histograms. Several “monitor” workspaces that appear do have the monitors but only for the last nxs file loaded, but we cannot tell for which run. Can we leave the monitor histogram workspaces grouped under the nxs workspaces??? When things go wrong one of the things we first look at are the monitor spectra.
comment:5 Changed 7 years ago by Peter Parker
Refs #8906 - More descriptive names for monitor workspaces.
Essentially, the name of the original workspace name with a "_monitors" suffix.
Changeset: 6df657415245e73c504d8cf4249c9e5434284e47
comment:6 Changed 7 years ago by Peter Parker
Refs #8906 - Move get_monitor method to Sample class.
Changeset: 96f039dace80e93a601d32cfe4ff6fe862a01045
comment:7 Changed 7 years ago by Peter Parker
Refs #8906 - SliceEvent now gets monitor from Sample class.
Changeset: a987a75b4953193c081a05e5d1e2a6995f6aa21d
comment:8 Changed 7 years ago by Peter Parker
Refs #8906 - Cleaner way to load monitors for event files.
Changeset: aa2c83f6f8a00025a39df32e9e162c1b682a99c6
comment:9 Changed 7 years ago by Peter Parker
Refs #8906 - Change tolerance account for different loading.
Changeset: 48f6e07702b59bf4562f795d732afb965fa50833
comment:10 Changed 7 years ago by Peter Parker
- Status changed from inprogress to verify
- Resolution set to fixed
Testing:
This one is best looked at by Gesner.
Gesner, the changes are as we discussed. Remember to merge in the system tests branch as well.
comment:11 Changed 7 years ago by Peter Parker
- Status changed from verify to reopened
- Resolution fixed deleted
Reopening after a discussion with Gesner.
comment:12 Changed 7 years ago by Peter Parker
- Status changed from reopened to inprogress
Refs #8906 - Use a scaled version of the same monitor.
Instead of passing the resulting monitor from SliceEvent to NormalizeToMonitor, just use a scaled version of the same monitor that was passed to SliceEvent.
These changes should also make the process of having monitors in a different time regime easier, which is an issue we will be facing in the future.
Changeset: bc772830e7f2c8f90f176a6fc16f6db755a3bfd6
comment:13 Changed 7 years ago by Peter Parker
Revert "Refs #8906 - Change tolerance account for different loading."
This reverts commit 48f6e07702b59bf4562f795d732afb965fa50833.
Since we now handle monitors correctly when normalising, the relaxed tolerance is no longer necessary.
Changeset: c168ba914290250d64fd2bd5935ac265b4b7442a
comment:14 Changed 7 years ago by Peter Parker
Refs #8906 - Only run scale when necessary.
Changeset: fffaee634aefa01580bc893f51207a3d391c4179
comment:15 Changed 7 years ago by Peter Parker
- Status changed from inprogress to verify
- Resolution set to fixed
comment:16 Changed 7 years ago by Peter Parker
- Status changed from verify to reopened
- Resolution fixed deleted
comment:17 Changed 7 years ago by Peter Parker
- Status changed from reopened to inprogress
Refs #8906 - Temp change to tol to fix system tests.
Until I can find out what's going on differently that's affecting LOQ.
Changeset: 2cc39538792789480eba3229fdfff27616e65b27
comment:18 Changed 7 years ago by Peter Parker
Revert "Refs #8906 - Temp change to tol to fix system tests."
This reverts commit 2cc39538792789480eba3229fdfff27616e65b27.
The problem actually originated in #8977.
Changeset: 373db072781861e1406c8b5c5d61c2e93bd1a57e
comment:19 Changed 7 years ago by Peter Parker
- Status changed from inprogress to verify
- Resolution set to fixed
comment:20 Changed 7 years ago by Gesner Passos
- Status changed from verify to verifying
- Tester set to Gesner Passos
comment:21 Changed 7 years ago by Gesner Passos
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/8906_improve_monitor_handling_in_isis_sans'
Full changeset: 2d7f5a626e29ca692cd4e18191de5c30ad834ea8
comment:22 Changed 7 years ago by Gesner Passos
- Keywords PatchCandidate added
The user requested to see the monitors of loaded data, and it was a 'non-wanted' feature introduced in 3.1. Hence, this should go 'published' ASAP.
comment:23 Changed 7 years ago by Nick Draper
- Milestone changed from Release 3.2 to Release 3.1.1
Moved to patch release 3.1.1
comment:24 Changed 7 years ago by Peter Parker
Refs #8906 - More descriptive names for monitor workspaces.
Essentially, the name of the original workspace name with a "_monitors" suffix.
Changeset: 35c50a7b79d3c0009bf369580800e3ac98726c43
comment:25 Changed 7 years ago by Peter Parker
Refs #8906 - Move get_monitor method to Sample class.
Changeset: 3dd15baf510bc09fd8a1dcb48b53440a367613b5
comment:26 Changed 7 years ago by Peter Parker
Refs #8906 - SliceEvent now gets monitor from Sample class.
Changeset: f6789b7d7df138aa8f68e25da2ec57d14a493f0e
comment:27 Changed 7 years ago by Peter Parker
Refs #8906 - Cleaner way to load monitors for event files.
Changeset: 6dce1bfc9d5692a0b93477bf4846b7e6b7dee5ce
comment:28 Changed 7 years ago by Peter Parker
Refs #8906 - Use a scaled version of the same monitor.
Instead of passing the resulting monitor from SliceEvent to NormalizeToMonitor, just use a scaled version of the same monitor that was passed to SliceEvent.
These changes should also make the process of having monitors in a different time regime easier, which is an issue we will be facing in the future.
Changeset: f122aaf02b514e460e2f090ee2702f8508c84d55
comment:29 Changed 7 years ago by Peter Parker
Refs #8906 - Only run scale when necessary.
Changeset: b4a09e4811598cc457e0f2cd387cd8f6b301917a
comment:30 Changed 7 years ago by Russell Taylor
The above 6 commits are from the rebase of this work onto the patch release branch.
comment:32 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 9749