Ticket #8020 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

The original data is corrupted when we get a loading error

Reported by: Arturs Bekasovs Owned by: Arturs Bekasovs
Priority: major Milestone: Release 3.2
Component: Muon Keywords:
Cc: anders.markvardsen@… Blocked By:
Blocking: #8547, #8860 Tester: Anders Markvardsen

Description (last modified by Arturs Bekasovs) (diff)

To reproduce:

  1. Open MuonAnalysis.
  2. Go to GroupingOptions tab and add a group with any name and detector IDs 1-50.
  3. Go back to Home, and load an attached EMU data file. It contains data for 96 detectors, so it should be loaded without any problems.
  4. Check that MuonAnalysis workspace contains data for 96 detectors.
  5. Now try to load AutoTestData/emu00006473.nxs file. It has data for 32 detectors only hence specified grouping could not be applied, which should be indicated by the error thrown.

Now, you should notice that none of the widgets (e.g the Description) has updated to reflect the information for the new file, which is expected as file loading failed. However, if you check the MuonAnalysis workspace, it contains data for 32 detector only, which means that it was overwritten and the new data will be used if you continue using the interface. This shouldn't happen.

The general problem is that we load the new file straight to the ws used by the interface. If the file was loaded but an error occurs on a later stage we don't go further to update the interface, but we have already overwritten the original data.

Attachments

EMU00036854.nxs (964.0 KB) - added by Arturs Bekasovs 7 years ago.
Data file for 96 detector EMU

Change History

Changed 7 years ago by Arturs Bekasovs

Data file for 96 detector EMU

comment:1 Changed 7 years ago by Arturs Bekasovs

That should be solvable by creating a tmp workspace and loading to it. If all the stages of the loading are successful we copy it to the ws used. If error occurs - we just discard it.

However, that involves moving around some significant amount of code. And, actually, the problem described is quite difficult to reproduce, so leaving it for later.

comment:2 Changed 7 years ago by Arturs Bekasovs

  • Description modified (diff)

comment:3 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 8547 added

(In #8547) There is a slight difficulty with that one: if we are not using the filename to determine the instrument, we can't actually say that the file user is trying to load is not a Muon file before actually loading it. This makes it much easier to reproduce the problem described in #8020. So it makes sense to do that other ticket first. Afterwards, I will be able to just throw an error if I determine that the instrument of the loaded workspace is not a Muon instrument and nothing will get affected.

comment:4 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 8547 removed

(In #8547) Trac playing tricks.

comment:5 Changed 7 years ago by Arturs Bekasovs

  • Blocking 8547 added

(In #8547) Trac playing tricks.

comment:6 Changed 7 years ago by Arturs Bekasovs

  • Description modified (diff)

comment:7 Changed 7 years ago by Arturs Bekasovs

  • Milestone changed from Backlog to Release 3.2

comment:8 Changed 7 years ago by Arturs Bekasovs

  • Summary changed from Muon: The original data is corrupted when we get a loading error to The original data is corrupted when we get a loading error

comment:9 Changed 7 years ago by Nick Draper

  • Status changed from new to assigned

Bulk move of tickets out of triage (new) to assigned at the introduction of the triage state

comment:10 Changed 7 years ago by Arturs Bekasovs

  • Blocking 8860 added

(In #8860) The interface currently loads the new data to the used workspace directly. It should be much easier when I load it to a tmp workspace. I will then just compare the number of detectors of the loaded one and the current one.

comment:11 Changed 7 years ago by Arturs Bekasovs

  • Status changed from assigned to inprogress

Refs #8020. Change loading code to use temporary workspace.

Temporary workspace is not in the ADS. When loading/correction/grouping are finished - temporary workspace is added to the ADS replacing MuonAnalysis and MuonAnalysisGrouped.

Changeset: 4f4cb0b302634a38e6841ffe3a1b5f828605def7

comment:12 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Remove redundant method.

We do not delete ranged workspaces as we are no longer storing them in the ADS.

Changeset: 711fdcbe10e3453e113cb36cca0a1be1432cd1cb

comment:13 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Re-enable support for a range of workspaces.

Changeset: 23285db2c26d48e45e9494c9b2d977a564115278

comment:14 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Fix DTC of multi-period workspaces.

Changeset: 1e8b5edd082c6ff111fe051b0fdaaec8a7522fea

comment:15 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Fix problem with groups not being replaced correctly.

ADS::addOrReplace is not replacing groups properly, which clutters up ADS quite badly.

Changeset: 4979db9aa13351dc87ad513174aeb9f5a531d012

comment:16 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Fix groupWorkspace() to handle ws which are already in ADS

Changeset: b73be60b3b2aee49fcb038a08e5bc5ddec60ff87

comment:17 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Make it possible to load after loading error.

Changeset: 1205de240e2115389ebb5a5fa4c136d223d31970

comment:18 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Add another version of groupWorkspace.

Previous version was trying to determine whether workspace is already in the ADS. It seems more natural to have two version of the function: one for when we need to group workspace which is already in the ADS and one to group newly created workspace.

Changeset: 0a84546ec7559d8858aa30b71a3187303460230b

comment:19 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Remove old and redundant code.

Current run loading is supported for ISIS only anyway, and current run of all the ISIS instruments is loaded from the file, not using LoadDAE. LoadDAE doesn't exist since 3.0 anyway, so if anybody would use the functionality - it would've been noticed.

Changeset: 39b475325e1ea122acb863113a461be4fb97ed00

comment:20 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Merge remote-tracking branch 'origin/master' into 8020

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/src/MuonAnalysis.cpp

Changeset: 65660d3e87c5cc1ace9944932ab9d586d805b8b7

comment:21 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Do various minor improvements.

Changeset: 1a58e26df83c94505a35e10578270894b54f0b5c

comment:22 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Add methods for loading files and applying DTC.

Changeset: 619d1d25d5f53933bfad40f8e3593ee9f7d106ae

comment:23 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Use new methods.

Changeset: f78bdda4d7268c3cb755c26a081627e287d2d4c4

comment:24 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Fix warning output.

Changeset: cd4998dbae8474a02e56bb7312c8e40355f7f504

comment:25 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Add instrument validation.

Changeset: ecfc685d70f311536d6b6cb11f122876f9b782c5

comment:26 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Refactor period retrieval code.

Changeset: 5ebcbf99452eaaf32622774566468729dc29248f

comment:27 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Move grouping code to separate method.

Lots of refactoring changes to make it possible.

Changeset: 3f6e3f27287005a745929ce64d2b18e361a8f67c

comment:28 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Refactor the method to use new functionality.

Changeset: 8b30c0fc677cddd19f3e17b1d1633e1b20869f57

comment:29 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Move run printing to other method.

Plus some other minor code movements.

Changeset: e16a5a91441cb0269cca971509cc9779a2a88a22

comment:30 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Better logging and error reporting.

Changeset: d3c234b052d57de508e74a369ef495e7c5c87eee

comment:31 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Better log messages and slight refactoring.

Changeset: 8bc8a08f294b466ebfcf56081c51bbeeaaa1196a

comment:32 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Fix the problem with grouping cleared when instr. changed

Changeset: 71bc0fe34ea0f61f0b059dbad1f372324c57f1cb

comment:33 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Catch logic_errors from LoadMuonNexus as well.

Changeset: d3a997f7b6270ffc349647488a5edfb181d6f2fe

comment:34 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Better error handling for DTC.

Changeset: d36a52d75901fdd15cb00ce6f1f784a7804160f6

comment:35 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Fix the MU character display.

Changeset: eeed785376c93010cdd4fd34217eca95583b979e

comment:36 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Merge remote-tracking branch 'origin/master' into 8020

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/inc/MantidQtCustomInterfaces/MuonAnalysis.h Code/Mantid/MantidQt/CustomInterfaces/src/MuonAnalysis.cpp

Changeset: 7e4722f6f876a45cc04975906c2a6b1f9bd15332

comment:37 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Move utility functions to MuonAnalysisHelper.

Changeset: 35d3b1d15014b686c9a894e3993783206a237e74

comment:38 Changed 7 years ago by Arturs Bekasovs

Tester:

That was a rather huge re-factoring excercise. Before the changes, the loading code was mixed up with interface state updating code. This meant that whenever an error occurs during the loading, the interface is left partly-updated.

What I've done here, is moved loading-correction-grouping code to separate functions. It allows me to run them, and if they are finished correctly I can feel safe about rewriting the current data. If they fail, I show an error and just discard the loading results.

The amount of changes means, however, that quite a few things should be tested here.

Basic loading

Load a few Muon files of various instruments, and verify that the interface gets updated correctly and you are able to plot/fit the data.

In particular, check that the following things are updated correctly:

  1. Time Zero and First Good Data (visible when From datafile is checked)
  2. Instrument and it's description. Use MUSR00015189 and MUSR00022725 from AutoTestData to check that MUSR orientation is loaded correctly - the first one should be longitudinal and the second one - tranverse.
  3. Number of periods in the file. Period widgets should be populated correctly.
  4. Run information.
  5. Grouping (see next section).
  6. Bin size on the Settings tab.

Grouping

There are three possible sources of loaded grouping:

  1. The one from IDF. Just load a few MUSR files to check that.
  2. If IDF grouping not available, grouping stored in the file. Remove Code/Mantid/instrument/Grouping/MUSR_Detector_Grouping_LF_64.xml and load e.g. MUSR00015189 to check that.
  3. If neither IDF grouping nor internal grouping is set, dummy grouping is used. Delete Code/Mantid/instrument/Grouping/ARGUS_Detector_Grouping_LF_192.xml and load any ARGUS files from AutoTestData (none of the have grouping set in the data file) to check that.

Dead time correction

Dead time correction should be applied. Change all the types of dead time correction and verify that the curve does get changed (by a tiny bit usually). Argus files do not have dead times in the data files, verify that it is handled correctly. Specify invalid custom dead time files to make sure it is handled correctly as well.

Errors

Check that the problem in the description is solved. Make sure that when error occurs, the interface is left unchanged, i.e. you can still work with the old data.

comment:39 Changed 7 years ago by Arturs Bekasovs

Refs #8020. Fix doxygen warnings.

Changeset: 61c0888d7f3c9fbbc6e864b94086eaabce0ab485

comment:40 Changed 7 years ago by Arturs Bekasovs

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:41 Changed 7 years ago by Anders Markvardsen

  • Status changed from verify to verifying
  • Tester set to Anders Markvardsen

comment:42 Changed 7 years ago by Anders Markvardsen

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/bugfix/8020_muon_data_damaged_on_load_error'

Full changeset: 6960cdde2ff763a93b6700ff8bb0f1dd90fbe7e5

comment:43 Changed 7 years ago by Anders Markvardsen

improved cleanup from user experience when error is thrown and improve code re-factoring

comment:44 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 8865

Note: See TracTickets for help on using tickets.