Skip to content
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

Add multiple time regime support for ISIS loading #18561

Merged
merged 2 commits into from Jan 30, 2017

Conversation

AntonPiccardoSelg
Copy link
Contributor

@AntonPiccardoSelg AntonPiccardoSelg commented Jan 26, 2017

These changes allow for multiple time regime loading of ISIS data. Our loading works correctly for files which have up to two time regimes. Rob has been using three time regimes which causes our LoadISISNexus2 algorithm to trip since it compares the number of detectors + monitors with the entry in NSP1 of the vms_compat. This comparison is not valid for this scenario, therefore the comparison is disabled when loading

Fixes #18560

To test:

Test that can load a multiple time regime file

  1. Take the file "LARMOR00009848.nxs" from the unit test repository. It contains three time regimes. If you try and load it into an old Mantid version without this fix, you should get an error
  2. Load the file
    • Confirm that the file can be loaded without any issues

That that can load a normal file where the NSP1 is incorrect

You can get the sample file from here: \olympic\Babylon5\Scratch\Anton\BAD_LOQ_FILE_WRONG_NSP1

  1. Get the file "LOQ74019.nxs" from the specified location. Note that I tampered with the file and changed the NSP1 entry. Once you have used it for testing, it might be best to delete it.
  2. Try to load it into Mantid
    • Confirm that this is not possible and an error occurs.

Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?

  • How do the changes handle unexpected situations, e.g. bad input?

  • Has the relevant documentation been added/updated?

  • Is user-facing documentation written in a user-friendly manner?

  • Has developer documentation been updated if required?

  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

@AntonPiccardoSelg AntonPiccardoSelg added Extra Attention Testers and Gate keepers should pay extra attention as this affects core aspects. State: In Progress labels Jan 26, 2017
@AntonPiccardoSelg AntonPiccardoSelg added this to the Release 3.10 milestone Jan 26, 2017
@DavidFair
Copy link
Contributor

Is this worth adding to the release notes?

@AntonPiccardoSelg
Copy link
Contributor Author

@DavidFair Good point. In principle yes, but there will be most likely follow up work during this release which will make this "obsolete".

@DavidFair
Copy link
Contributor

Ok - I completed the tests locally so I'm happy to :shipit:

@peterfpeterson peterfpeterson merged commit 7b5062c into master Jan 30, 2017
@peterfpeterson peterfpeterson deleted the 18560_enable_multiple_time_regime_loading branch January 30, 2017 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extra Attention Testers and Gate keepers should pay extra attention as this affects core aspects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable file loading for multiple file regimes
3 participants