Ticket #11421 (closed)
Fix time chunks in ISIS direct inelastic
Reported by: | Alex Buts | Owned by: | Alex Buts |
---|---|---|---|
Priority: | critical | Milestone: | Release 3.4 |
Component: | Direct Inelastic | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Tester: | Federico Montesino Pouzols |
Description (last modified by Alex Buts) (diff)
When ISIS direct inelastic reduction works in multirep mode, it cuts workspace in units of TOF into chunks according to TOF ranges, from fastest TOF on closest(to sample) detector to slowest TOF on furtherest detector for the selected energy range.
doing so it currently does not take in consideration the fact, that the TOF counter starts counting somewhere at the moment pulse hits the moderator, and calculates TOF range assuming that TOF-0 occurs when pulse hits monitor 2. This causes incorrect selection of the TOF range and incorrect cut-off of energy transfers on closest detectors (see picture attached, the black ark-shaped area in the left bottom corner)
The issue has to be fixed.
Attachments
Change History
comment:4 Changed 6 years ago by Alex Buts
Re #11421 Warn user if tof_spectra are not defined in IDF
also touch changed parameter files for them to work on developer mantid
Changeset: 2c372796b06ff62be93751e56beb1f0cc9d84d6a
comment:5 Changed 6 years ago by Alex Buts
Re #11421 Provide more accurate calculations for energy range
Changeset: 0be35b95e83ad2a5b34b85f7d13ca9f4e71904cd
comment:6 Changed 6 years ago by Alex Buts
Re #11421 Better processing for range of the filenames
Changeset: 96df3219c6cb5892d6acc61cc3ebb80dbefc3ef4
comment:7 Changed 6 years ago by Alex Buts
Re #11421 Fixed "invalid tuple" error when target directory is not
available.
Changeset: b995db13611faeef845f4d54f59d6a047b09dd37
comment:8 Changed 6 years ago by Alex Buts
Re #11421 minor Comments
Changeset: dd268d223b02f826b23cefb7988fbc807f83b2c5
comment:9 Changed 6 years ago by Alex Buts
Re #11421 Modified reference file for LETMulirep system test
Changeset: d47b2974cd2f0cad1c0ee70209d23ec6036c789f
comment:10 Changed 6 years ago by Alex Buts
Re #11421 Enabled clearing old results in multirep mode
and various unit tests for ReductionWrapper, verifying output file name procedure
Changeset: 26cddbd226f53bb27fd3a663b5659dc26d2a2082
comment:11 Changed 6 years ago by Alex Buts
it addressed as PR https://github.com/mantidproject/mantid/pull/464
comment:12 Changed 6 years ago by Alex Buts
It is not easy to test this ticket as the reason for the problem appeared not the one initially anticipated. I think, testing this by code review is the most appropriate way forward.
Actually, it was wrong instrument configuration causing the problem -- not error in the chunking procedure. The chunking procedure was occasionally working correctly as IDE counter starts counting approximately at the pulse at source.
This ticket was used as opportunity to make program independent on the quality of this assumption, to fix two other minor bugs identified by instrument scientists during the run and to write more unit tests to confirm the code correctness.
To check, that the problem do not appear any more, a careful tester may run LETMiltirep2015 test, try to produce picture as above (use SofQ3 and Sliceviewer to do that in Mantid) and ensure that no missing corners appear. As the problem was due to the instrument configuration rather then bug in the program, previous instance of Mantid with correct instrument would not reproduce the problem as well.
I did this check number of times working on the ticket so a tester may trust my judgement on this (I will be called overnight if this appear in any way incorrect)
There were also changes to ensure that previously reduced workspaces are deleted when multiple files are processed not to grab increasing amount of Mantid memory. This feature is unit tested for ticket and verified on working instrument too.
comment:13 Changed 6 years ago by Alex Buts
Re #11421 Better diagnostics message if chunking can not identify
proper incident energy
Changeset: 5025671e0e3bb857a16e44038b2ba2a56c262914
comment:14 Changed 6 years ago by Federico Montesino Pouzols
Is this PR for release 3.3.1? Or is it 3.4?
comment:15 Changed 6 years ago by Federico Montesino Pouzols
- Status changed from inprogress to verifying
- Tester set to Federico Montesino Pouzols
comment:16 Changed 6 years ago by Federico Montesino Pouzols
I'm not sure what will happen with 3.3.1. I'd assume next release will be 3.4, as the freeze will be around the 10th of April if I recall correctly.
Let's merge it in, as it all looks good, tests passed locally, and it is a required fix. I run LETReductionEvent2015Multirep and it worked well. I couldn't go much farther. If you'd like me to double-check/reproduce the SofQ3/sliceviewer figure just let me know, as I wasn't sure what exact files and steps are involved. Maybe a check for the "missing corner" and/or similar glitches could be added in the system test later on?
I only noticed that the system test LETReductionEvent2015Multirep could run with much less than 20GB, I think that 16 or 10GB would be enough, as it used less than 10GB on debian when I run it (not sure if for some reason it's more memory hungry on windows). Decreasing this limit in requiredMemoryMB() would ensure that the test is run more often and not skipped on most/many machines.
comment:17 Changed 6 years ago by Alex Buts
Re #11421 Warn user if tof_spectra are not defined in IDF
also touch changed parameter files for them to work on developer mantid
Changeset: 2c372796b06ff62be93751e56beb1f0cc9d84d6a
comment:18 Changed 6 years ago by Alex Buts
Re #11421 Provide more accurate calculations for energy range
Changeset: 0be35b95e83ad2a5b34b85f7d13ca9f4e71904cd
comment:19 Changed 6 years ago by Alex Buts
Re #11421 Better processing for range of the filenames
Changeset: 96df3219c6cb5892d6acc61cc3ebb80dbefc3ef4
comment:20 Changed 6 years ago by Alex Buts
Re #11421 Fixed "invalid tuple" error when target directory is not
available.
Changeset: b995db13611faeef845f4d54f59d6a047b09dd37
comment:21 Changed 6 years ago by Alex Buts
Re #11421 minor Comments
Changeset: dd268d223b02f826b23cefb7988fbc807f83b2c5
comment:22 Changed 6 years ago by Alex Buts
Re #11421 Modified reference file for LETMulirep system test
Changeset: d47b2974cd2f0cad1c0ee70209d23ec6036c789f
comment:23 Changed 6 years ago by Alex Buts
Re #11421 Enabled clearing old results in multirep mode
and various unit tests for ReductionWrapper, verifying output file name procedure
Changeset: 26cddbd226f53bb27fd3a663b5659dc26d2a2082
comment:24 Changed 6 years ago by Alex Buts
Re #11421 Better diagnostics message if chunking can not identify
proper incident energy
Changeset: 5025671e0e3bb857a16e44038b2ba2a56c262914
comment:25 Changed 6 years ago by Federico Montesino Pouzols
- Status changed from verifying to closed
Merge pull request #464 from mantidproject/11421_tof_range_calculations
Correct TOF range in multirep mode
Full changeset: 8e2b160c7983e9ece7f7a67c0a793b99eb8e6be3
comment:26 Changed 6 years ago by Stuart Campbell
The only changes we had for 3.3.1 were to make mantid work on RHEL7 - but it seems with the code freeze so close we won't bother with a 3.3.1 release now - unless @martyngigg would like a formal rhel7 3.3 kit ?
comment:27 Changed 6 years ago by Martyn Gigg
I agree I don't think we need to bother with 3.3.1 now. The only people wanting rhel7 needed a nightly anyway so we can just keep going with that.
comment:28 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 12260