Ticket #9873 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

Fix path handling in SANS isis_reductions_steps

Reported by: Martyn Gigg Owned by: Peter Parker
Priority: critical Milestone: Release 3.3
Component: SANS Keywords:
Cc: Blocked By:
Blocking: Tester: Anders Markvardsen

Description

The path handling in scripts/SANS/isis_reduction_steps.py is inconsistent. Some places use os.path.join and others simply add / separators. These all should harmonize on using os.path.join.

Another related problem is path handling when loading the pixel correction file on line 1581 of scripts/SANS/isis_reduction_steps.py. The load is done via an {{{eval}} call where all of the code is in a string. If the self._pixel_file is a windows path that happens to contain a directory name that starts with a character that can be interpreted as an escape character, e.g.

C:\release\Data\LOQ/FLAT_CELL.061

where the \r would be interpreted as a carriage return then the path is invalid and the command fails. In this particular case I think line 1581 just needs to be changed to

load_com = self._load+'(Filename=r"'+self._pixel_file+'",OutputWorkspace="'+pixel_adj+'"'

to make the Filename string a raw string so that escape characters are ignored.

Change History

comment:1 Changed 6 years ago by Peter Parker

Refs #9873 - Change to os.path.join.

Changeset: 9606259281c951eb5d4d4f8b5e33f424d5b3cefe

comment:2 Changed 6 years ago by Peter Parker

Refs #9873 - Remove use of eval().

The comments in the previous code indicated that CalculateNormISIS could be inherited from and that base classes could potentially want to override _load and _load_params. This has not been done anywhere in the code, and I cannot see a ticket that would require it, so I'm just removing the complex string concatenation and replacing it with a simple call to LoadRKH, which is the algorithm used to load pixel correction files.

Changeset: 5694d58c024cc996f6c44ed2a396ab818dfe4e7d

comment:3 Changed 6 years ago by Peter Parker

Note that the LOQMinimalBatchReduction system test covers the change to the use of eval.

comment:4 Changed 6 years ago by Peter Parker

  • Status changed from new to assigned

comment:5 Changed 6 years ago by Peter Parker

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

To test, a code review is probably enough here.

If you like, you could try renaming your system tests folder to release or similar and see if LOQMinimalBatchReduction still passes.

comment:6 Changed 6 years ago by Anders Markvardsen

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

comment:7 Changed 6 years ago by Anders Markvardsen

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/bugfix/9873_path_handling_in_isis_reduction_steps'

Full changeset: 43a11947fa020db415f8dea5179c12dada56b552

comment:8 Changed 6 years ago by Anders Markvardsen

looks good - got rid of eval

comment:9 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 10715

Note: See TracTickets for help on using tickets.