mantid icon indicating copy to clipboard operation
mantid copied to clipboard

Modify ISIS SANS code to no longer use deprecated import

Open peterfpeterson opened this issue 1 year ago • 2 comments

There are various places in the code that import the wrong package and generate the warning

This ISIS Command Interface is deprecated.
Please change 'import ISISCommandInterface' or 'from ISISCommandInterface'to use 'sans.command_interface.ISISCommandInterface' instead.

This is an unnecessary warning that is generated by the following files

$ rg "import ISISCommandInterface" -t py -l
scripts/test/SANS/SansIsisGuiSettings.py
scripts/test/SANS/SANSReducerTest.py
scripts/test/SANS/SANSCentreFinderTest.py
scripts/test/SANS/SANSCommandInterfaceTest.py
scripts/test/SANS/SANSReductionStepsUserFileTest.py
scripts/SANS/ISISCommandInterface.py
Testing/SystemTests/tests/framework/ISIS/SANS/SANS2D/SANS2DSearchCentreGUI.py
Testing/SystemTests/tests/framework/ISIS/SANS/WORKFLOWS/SANSMergedDetectorsTest.py
Testing/SystemTests/tests/framework/ISIS/SANS/SANS2D/SANS2DReductionGUIAdded.py
Testing/SystemTests/tests/framework/ISIS/SANS/SANS2D/SANS2DSlicing.py
Testing/SystemTests/tests/framework/ISIS/SANS/SANS2D/SANS2DReductionGUI.py
Testing/SystemTests/tests/framework/ISIS/SANS/WORKFLOWS/SANSLoadersTest.py
Testing/SystemTests/tests/framework/ISIS/SANS/LOQ/SANSLOQAddBatch.py
Testing/SystemTests/tests/framework/ISIS/SANS/LOQ/SANSLOQReductionGUI.py

Worse still is that several of these imports rename the package to a poorly chosen name

import ISISCommandInterface as i

where a clearer option has already been suggested in some of the files

import ISISCommandInterface as command_iface

peterfpeterson avatar Jul 13 '23 13:07 peterfpeterson

As part of this we should update the documentation for the scripting interface, as that is still showing examples that include the deprecated import: https://docs.mantidproject.org/nightly/techniques/ScriptingSANSReductions.html.

Also, when the import is used in Mantid Workbench I'm not seeing the warning logged in the messages pane - I'm not sure if this is supposed to be the case or not. We had a scientist that tried to use the deprecated import when loading a TOML user file, which is only supported by the newer import, but they were unable to resolve this themselves because the warning about the deprecated import doesn't seem to display.

rbauststfc avatar Mar 18 '24 14:03 rbauststfc

After an initial investigation by @yusufjimoh, we think this one is a bit more complex than it initially appears. For example, if you change the import in scripts/test/SANS/SansIsisGuiSettings.py then the new version of the interface has a different implementation that causes the tests to break. Although the deprecated version of the interface throws a warning on import, I can't actually see anything about the switch to the new interface in our documentation and all examples in the docs currently use the old import.

I think this issue will involve investigating the background of the deprecation more fully, including with our scientists, and checking if we still need to support the deprecated version or if it could now be removed (if not then test coverage would still be required). It would also involve making sure that documentation is updated appropriately and that there is appropriate test coverage of the new version of the interface.

rbauststfc avatar Jun 25 '24 11:06 rbauststfc