Ticket #9104 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

Loading a muon file in python creates unwanted output

Reported by: Roman Tolchenov Owned by: Arturs Bekasovs
Priority: major Milestone: Release 3.2
Component: Framework Keywords: CORE
Cc: Blocked By:
Blocking: Tester: Martyn Gigg

Description

ws = Load('MUSR15189') creates the data workspace and the grouping table named LoadChildWorkspace.

Loading the same file from the GUI creates only the data.

Change History

comment:1 Changed 7 years ago by Owen Arnold

  • Status changed from new to assigned

comment:2 Changed 7 years ago by Nick Draper

  • Owner set to Arturs Bekasovs

comment:3 Changed 7 years ago by Arturs Bekasovs

LoadMuonNexus has two optional output workspace properties. In the algorithm code, these are returned only if their name is set to something. So it's fine when you run

ws = LoadMuonNexus("MUSR15189") 

Though the generic Load algorithm sets LoadChildWorkspace to all the unset output workspace properties (to make validator happy), even if the workspace is optional, which causes the described behaviour.

The problem could be solved by fixing Load algorithm to check if the WorkspaceProperty is optional first. I've tried that quickly and it seem to work, though I need somebody more senior to take a look at that.

comment:4 Changed 7 years ago by Arturs Bekasovs

This doesn't happen if we run it from the dialog, because the dialog sets the workspace properties specifically to input value (which is "" if we leave them empty), after they were set to LoadChildWorkspace (which happens when the filename property is changed). Python caller doesn't do that, neither does C++ invocation of Load, which causes LoadChildWorkspaces to be created.

Last edited 7 years ago by Arturs Bekasovs (previous) (diff)

comment:5 Changed 7 years ago by Arturs Bekasovs

  • Status changed from assigned to inprogress

Refs #9104. Don't set up optional output ws properties.

Changeset: 748abbddb496ee674f1878a9131ab06b5050f3c3

comment:6 Changed 7 years ago by Arturs Bekasovs

Tester:

Somebody senior please, core stuff.

Please make sure you understand the implications behind the change (I've tried to explain my understanding in previous comments) and that the code update is reasonable and not dangerous. Obviously, system/unit tests should be passing.

Then verify that the additional workspaces are not created when loading Muon file both using Load from Python and load dialog.

comment:7 Changed 7 years ago by Arturs Bekasovs

An issue related to this one: createChildAlgorithm also sets output workspaces to ChildAlgOutput no matter what their isOptional() status is.

Last edited 7 years ago by Arturs Bekasovs (previous) (diff)

comment:8 Changed 7 years ago by Arturs Bekasovs

createChildAlgorithm case is more complicated than the Load one, as suggested by Martyn, so not touching it in this ticket.

Additionally, tests should be added for the Load to check that it is now behaving correctly when optional output workspaces

  1. are requested
  2. are not requested

comment:9 Changed 7 years ago by Arturs Bekasovs

Refs #9104. Tests for new functionality

Changeset: 3d9e6c238dd550af4a3acef4075997fb92102f03

comment:10 Changed 7 years ago by Arturs Bekasovs

Tester:

See comment:6.

Additionally, please verify the added tests.

comment:11 Changed 7 years ago by Arturs Bekasovs

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

comment:12 Changed 7 years ago by Arturs Bekasovs

  • Keywords CORE added

comment:13 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to verifying
  • Tester set to Martyn Gigg

comment:14 Changed 7 years ago by Martyn Gigg

I think leaving createChildAlgorithm alone is the correct thing to do as it's a simple utility function that sets up the algorithm appropriately in 99% of circumstances. A caller is free to override what it has created anyway.

comment:15 Changed 7 years ago by Martyn Gigg

The code change looks fine and I confirm that now in Python you only get the single output workspace if that was all you specified but you get the extra ones if you have requested them.

comment:16 Changed 7 years ago by Martyn Gigg

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/feature/9104_load_option_output_ws'

Full changeset: eac5434cb5188934659e449ddcf95184b14d0d1d

comment:17 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 9947

Note: See TracTickets for help on using tickets.