Loader for SANS-1 at MLZ

[AndriiDemk]

Description of work.

This PR contains the loader for SANS-1 datafiles. The loader makes it possible for Mantid to manage the files produced by the SANS-1 instrument at MLZ (ASCII files with extensions .001) and reduced files with extension .002.

Pull request consists of 3 major files:

1. LoadSANS1MLZ.py (algorithm)
2. SANS1DataMLZ.py (helper file to process and structure data)
3. LoadSANS1MLZTest.py (tests for processing and loading data into Mantid)

as well as algorithm documentation and supplemental files.

To test:

• Load a test file ( such as UnitTest/D0122881.001 )
• Open the instrument viewer and check if the view works as expected (especially in 3D)

Fixes #34267.

---

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?
• Are the unit tests small and test the class in isolation?
• If there is GUI work does it follow the GUI standards?
• If there are changes in the release notes then do they describe the changes appropriately?
• Are the release notes saved in a separate file, using Issue or PR number for file name and in the correct location?

Functional Tests

• Do changes function as described? Add comments below that describe the tests performed?
• Do the changes handle unexpected situations, e.g. bad input?
• Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

[perenon]

Hello, Sorry for the delay. We reviewed your pull request with @MathieuTillet @gui-co @eurydice76 and Miguel. We have numerous comments.

1- The most important comment here is that you created an algorithm but not a loader

• Loader in Mantid are similar to algorithms, but can be accessed directly through a general load() method.
• As far as we know, a loader must be written in C++, inherit from interface IFileLoader, call the DECLARE_FILELOADER_ALGORITHM macro and also implement a method confidence() for declaring a "confidence score" to express its compatibility with a given file.
• We strongly suggest to re-write the loader code in C++, which unfortunately represents a large amount of work.
• We also suggest you to check the LoadSNSSpec (https://github.com/mantidproject/mantid/blob/main/Framework/DataHandling/src/LoadSNSspec.cpp) loader which is, like yours, an ASCII loader.

2- The tests If the loader should be refactored to C++, the test part can be kept as it is. We just have some comments:

• You do not provide any *.002 file in the test files
• The generated files in your test are not always removed after the test, especially if a failure or an error occurs before the call to os.remove(). Implementing tearDown() method may be the nice way to remove files after testing.
• It was unclear how you set up the data search directories. We assume you use the method set_up (line 210). We recommend to use os.path.join instead of + operator. We also do not understand what happens if another UnitTest folder exists in your directory.
• The choice of creating numerous small classes (such as SANS1DataClassCounterSectionTest) appears to be a bit heavy, but can also be useful if you decide to extend the behavior of the loader.
• Algorithms can be imported directly through import mantid.simpleapi, so using run_algorithm() is not mandatory

3- Use of matrix mode

• Your propose a matrix mode to store data in 2D, then enabling visualization with Slice Viewer => this is not the philosophy of Mantid. Mantid uses the second matrix dimension to store the "TOF" dimension. The vector mode seems the most Mantid-adapted one.
• We think the matrix mode will not be able to be used if you extend your loader to TOF mode

4- General comments on the loader

• line 167: you can test if the value user_wavelength is set to default instead of testing its value compared to a float. The method for a property is isDefault()
• line 205: it is disturbing to raise a FileNotFoundError after catching a ValueError
• line 285: we think, as you mentioned, that the data_x generation will not work if you use TOF data (n>1). Notably, the length of data_x should be self.spectrum_amount()+1
• We warn you that the value of Beamstop.setup is not close to (0,0), which we think comes from the fact that the coordinates are not the image coordinates (where the center of image is (0,0)
• The loader should contain more comments

We can continue this discussion on Github, email or even by videoconf if you want.

Regards,

[mganeva]

@perenon Remi, thanks a lot for the review and for your comments. Unfortunately, we will have to postpone rewriting the loader in C++, since Andrii's internship ends at the end of September.

Andrii will fix other issues you have mentioned, but we will stay with the Python algorithm for now. It is important to get this PR finished till the end of September. So, our SANS-1 instrument scientists will be able to load their data to Mantid and start the evaluation of ILL SANS data reduction.

Later we can replace the Python loader with a C++ one.

[perenon]

We totally get that. Just remember that your code is an algorithm and not a loader! In that case an important question is: do you need to merge this PR (i.e. do you have to possibility to build a local version for your needs) ? I think once merged, your algorithm may stay in Mantid (I understood they could not be removed for compatibility purposes), which will be confusing if you refactor it in C++.

Cheers, Remi

[AndriiDemk]

Hello, Thank you a lot for the review and for such detailed comments. I just finished implementing your suggestions in the code review. Below is a short summary of the changes that I made.

1. The most important comment here is that you created an algorithm but not a loader
   • Discussed in Marina's comment
2. The tests
   • The support of .002 extension was removed from the algorythm, because we decided not to include in the loader support for processed files.
   • All of the other suggestions were implemented.
3. Use of matrix mode
   • “Matrix” mode was completely removed from the algorithm to comply with the philosophy of Mantid.
4. General comments on the loader
   • line 167: you can test if the value user_wavelength is set to default instead of testing its value compared to a float. The method for a property is isDefault() - implemented
   • line 205: it is disturbing to raise a FileNotFoundError after catching a ValueError - thanks, I modified it
   • The loader should contain more comments - I included several additional comments

[AndriiDemk]

• line 285: we think, as you mentioned, that the data_x generation will not work if you use TOF data (n>1). Notably, the length of data_x should be self.spectrum_amount()+1

Currently, the length of the data_x is 2 * self.spectrum_amount(). Could you please explain why it should be changed to self.spectrum_amount() + 1 ?

[AndriiDemk]

• We warn you that the value of Beamstop.setup is not close to (0,0), which we think comes from the fact that the coordinates are not the image coordinates (where the center of image is (0,0)

I do not think I am using the values that Beamstop.setup takes anywhere in the algorithm, I just read them out. Or I am missing something?

[mganeva]

@perenon Remi, sorry, I have missed your message. It is important for us to get this PR merged. "Local mantid versions for particular needs" are, to my opinion, not the way to go. It is also more time-consuming to maintain and to distribute them.

I also don't see any confusion if we will rewrite this algorithm in C++ in the future, to be honest: the name of the algorithm and the interface will stay the same. Thus, this change will be barely noticeable for the user.

As far as I remember, there is also a procedure in Mantid to remove the unneeded algorithms. First, you declare them as deprecated for a few releases and then remove. Thus, I really don't see any problem in merging of this PR.

[perenon]

Hello everyone!

Concerning the question about merging or not, it was just a (maybe clumsy) way to say that at some point you will have to go through a similar process in a near future. I also would always try to avoid the suppression of functions in a API, but I totally understand your point.

If it is OK for you, I check the modification before the end of this week (I will try to do it this afternoon).

Cheers, Remi

[perenon]

Hello, Some feedback:

• We think the length of the data_x should be self.spectrum_amount()+1 instead of 2*self.spectrum_amount(), because you need to have N+1 values of "X" if you have N bins in "X". However, this is a discussion for TOF implementation.
• About the value of the Beamstop.setup: You can find it in Show Sample Logs => Setup.BeamstopX and Setup.BeamstopY this is a value that may have been extracted from your data, whose coordinates are not close to (0,0) -(495,497) in your example-. I think that as long as you are aware of that, that should be OK.
• On last point, there is no entry in a Changelog or something for these modifications. I think the gatekeeper (next reviewer) will point you the proper file to modify.

[DannyHindson]

I can merge this PR today. The discussion about Loader\Algorithm seems OK to me. The MLZ team seem to be happy that it won't be available via Load (at least not yet). Also I think the algorithm list in Mantid is not so pure\clean that having an additional algorithm that may be removed at some point will cause any problems. @AndriiDemk - are you happy if I squash the commits when I merge this? We normally do if there's ~50+ commits

[AndriiDemk]

Sure, it works for me, thank you