37795 change polaris pdf norm

[andy-bridger]

Description of work

Summary of work

As per issue #37795 added an option for the vanadium normalisation to be done either using relative or absolute normalisation.

Fixes #37795.

Further detail of work

This is done by addition of a new mode "pdf_norm". In order to accommodate this some additional alterations were required to the enum class and the test files. An additional test was added to check the absolute normalisation.

To test:

Run the ISISPowder_PolarisTest.py tests. There should now be 18. The new tests FocusTestNoAbsorptionWithRelativeNormalisation and FocusTestNoAbsorptionWithAbsoluteNormalisation have replaced FocusTestNoAbsorption.

---

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

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?
• Do the release notes conform to the release notes guide?

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.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

[RichardWaiteSTFC]

Thanks for this, just a minor thing and a request to tidy up a function which you didn't write (but seeing as you are in the file anyway).
Also would it be possible to get a release note for this - we will want users to be aware of it (to that end we will also contact POLARIS scientists when it's merged)!

[RichardWaiteSTFC]

Looks like these functions are also in FocusTestNoAbsorptionWithRelativeNormalisation - would it be possible to define them as helper functions once outside the test classes so they don't have to be duplicated?

[andy-bridger]

I've done this - I wasn't sure what variable to use for the passed class objects as self seemed inappropriate for a general helper function - so I called it test.

[RichardWaiteSTFC]

Sorry I just meant the two functions generate_error_message and assert_output_file_exists being moved to external helper functions - but I now see the latter requires the test object.

It is annoying with systems test that each test is it's own class, I have not seen the test class being passed in this way before but it works!

It might be overkill, put one possible solution is to define a new base class with these methods like so

class focus_system_test_helper(systemtesting.MantidSystemTest): 
  def _assert_output_file_exists(directory, filename):
        test.assertTrue(os.path.isfile(os.path.join(directory, filename)), msg=generate_error_message(filename, directory))
    
  def _assert_focus_output_files(ws_num):
        user_output = os.path.join(output_dir, "17_1", "Test")
        assert_output_file_exists(user_output, f"POLARIS{ws_num}.nxs")
        assert_output_file_exists(user_output, f"POLARIS{ws_num}.gsas")
        output_dat_dir = os.path.join(user_output, "dat_files")
        for bankno in range(1, 6):
            assert_output_file_exists(output_dat_dir, f"POL{ws_num}-b_{bankno}-TOF.dat")
            assert_output_file_exists(output_dat_dir, f"POL{ws_num}-b_{bankno}-d.dat")

# outside class
def _generate_error_message(expected_file, output_dir):
      return f"Unable to find {expected_file} in {output_dir}.\nContents={os.listdir(output_dir)}"

Then define the individual classes like so

class FocusTestNoAbsorptionWithAbsoluteNormalisation(focus_system_test_helper):

I'm pretty sure the test suite won't try to run the new base class, if you have time feel free to give it a go, otherwise happy to approve!

[andy-bridger]

It seems it does try and run the helper class, I guess because in inherits from systemtesting.MantidSystemTest. I can get around it by putting it within another class which doesn't inherit. This work around allows more of the repeated code to be moved to the helper but is perhaps a bit hackier? I've added it as a commit, but let me know and we can roll back to the previous format

[RichardWaiteSTFC]

Oh dear sorry! That does seem to be a bit hacky - one could also set the base class test to be skipped by overloading the skipTests method and returning True - but then this would be recorded as a skipped test!
If it's OK with you let's roll back and stick with what you had!

[andy-bridger]

Yeh, I saw that as an option but also didn't like the fact it would come up as a skipped test. Should be reverted now

[RichardWaiteSTFC]

I'm OK to have an additional file, but is there a reason why you couldn't use 98533 and just have a different reference file?

[andy-bridger]

Short answer is probably no, I'm sure there is a way that could've been done. I just tried to keep it consistent and it seemed that the old test took 98532 and ran the normalisation then compared it to 98533, so I figured we could run a different normalisation and compare it to 98534.

[andy-bridger]

I think all those comments have now been addressed?

[RichardWaiteSTFC]

One more thing that I should have thought about before sorry!
Can we update the Polaris reduction docs here?

mantid/docs/source/techniques/ISISPowder-Polaris-v1.rst

Line 361 in f81538d

Accepted values are: **PDF** or **Rietveld**

[RichardWaiteSTFC]

Looks good thanks for the changes!

[RichardWaiteSTFC]

Actually if you get chance could you clean up the commit history? Specifically instead of adding another separate commit to revert the previous one, could you do git rebase -i HEAD~4 and remove the commit(s)
a4f39ff
e0697c5

There may be another way in git, but that's what I use! It's good practice to keep it clean (I know it's my fault for suggesting changes that don't pan out!)

[andy-bridger]

Sure, I did have a think about it before reverting and came to the conclusion that I wanted to have it in the commit history in case we changed our minds again (to still have the code accessible), but if you're happy for it to go, I am too

[RichardWaiteSTFC]

Sure, I did have a think about it before reverting and came to the conclusion that I wanted to have it in the commit history in case we changed our minds again (to still have the code accessible), but if you're happy for it to go, I am too

Thanks, if you could go ahead that'd be great.
Btw, I see the logic in wanting to keep a record of the change in case you need it later, in that instance you can always make a new branch at HEAD before doing changes to commit history on this PR branch if you want to.