Ticket #8996 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

PythonInterface casting for workspaces can't work with multiple inheritance, e.g IMaskWorkspace

Reported by: Martyn Gigg Owned by: Martyn Gigg
Priority: critical Milestone: Release 3.2
Component: Python Keywords: CORE
Cc: Blocked By:
Blocking: #8304, #8943 Tester: Owen Arnold

Description

It was found while trying to implement #8943 that the automatic downcast of DataItem pointers when returning to Python doesn't work for objects that multiple inherit from several interfaces.

The current mechanism tries to determine the required Python class automagically but fails in these types of circumstance and due to how boost python works it can't be made to work.

This ticket will be about implementing a simpler solution than the auto casting. It will simply have a C++ map of string IDs to the C++ types that they should be exposed as when returned to Python. This will also allow us more flexibility in choosing what is returned.

Change History

comment:1 Changed 7 years ago by Martyn Gigg

  • type changed from defect to enhancement

comment:2 Changed 7 years ago by Martyn Gigg

  • Status changed from new to inprogress

Remove old DowncastReturnedValue policy

It is replaced by ToWeakPtrWithDowncast & ToSharedPtrWithDowncast policies. The DowncastRegistry now maps DataItem::id strings to the interfaces classes that the DataItem obejects should be cast to when returning to Python Refs #8996

Changeset: 7313892660ddffe54352312b34948f8762377c9e

comment:3 Changed 7 years ago by Martyn Gigg

Add a class to do all required registration for a DataItem subclass

It encapsulates everything required for pointer & downcast registration. Refs #8996

Changeset: 284788c3eb7910c752e8b4c43700226b05668f4e

comment:4 Changed 7 years ago by Martyn Gigg

Remove the SharedPtrToPython macro.

It's unnecessary now as a direct boost::python call is clearer. Refs #8996

Changeset: 9469062edce0f4e3ffdd8e540e370cdb4e6ef198

comment:5 Changed 7 years ago by Martyn Gigg

Remove DowncastReturnedValue from CMakeLists.txt

Refs #8996

Changeset: 0b4e5a52ef0c69c243dabb11a5344bcb59defb16

comment:6 Changed 7 years ago by Martyn Gigg

Remove SharedPtrToPythonMacro from CmakeLists

Refs #8996

Changeset: f0a5a15b980500af912c6be9bd6d283a3eff1f64

comment:7 Changed 7 years ago by Martyn Gigg

Refactor TypeRegistry to use a static class like the DowncastRegistry.

Refs #8996

Changeset: 0049beee16e6bb80862ef751b35495b0f834ee05

comment:8 Changed 7 years ago by Martyn Gigg

Use new RegisterDataItemInterface class in place of macro

This now does the required additional registration for workspace types. Refs #8996

Changeset: ab9b044da683d3cfd0edb08020516af2190dedda

comment:9 Changed 7 years ago by Martyn Gigg

Put back ADS raising KeyError rather than RuntimeError.

Refs #8996

Changeset: 392baa6fa3d8086141f1d67d792b7ce485e921bf

comment:10 Changed 7 years ago by Martyn Gigg

Remove redundant code.

The SingleValueTypeHandler is now replaced everywhere by the TypedPropertyValueHandler. Refs #8996

Changeset: 5890b30da2ed9894fc06a8b5ee965b7a585e7a0a

comment:11 Changed 7 years ago by Martyn Gigg

Tidy up PropertyWithValueFactory.

Remove macro usage and store shared_ptrs in map. Refs #8996

Changeset: d6c7c574674a2a7d95c2a93a1661eb7748db671a

comment:12 Changed 7 years ago by Martyn Gigg

Dynamic cast when setting property from weak_ptr in python

This accounts for multiple-inheritance scenarios where a static_cast cannot detect that a cast is actually possible. Refs #8996

Changeset: 824298bc8a4b921cad3ff8b96fc157b0853c0cbc

comment:13 Changed 7 years ago by Martyn Gigg

Add missing include for exception type.

Refs #8996

Changeset: 56be19ec69a53961cf7c34dce1fceb9fa27a5e7f

comment:14 Changed 7 years ago by Martyn Gigg

Fix type for map insertion in TypeRegistry

Refs #8996

Changeset: c1b0b91fa3f5ac6f971b4f2f0b87eecc328708e4

comment:15 Changed 7 years ago by Martyn Gigg

Fix Python function tests.

They were calling getProperty().value on a non-child algorithm where the internal workspace pointer is null. Refs #8996

Changeset: 2adda60aceb1c8b21a58d4594c875e3fe34af5d6

comment:16 Changed 7 years ago by Martyn Gigg

Check for null pointer in DowncastingPolicies.

The return is now just the PyNone object. Refs #8996

Changeset: 550d68c35e0d91e4801116927787f5860d4bb0ec

comment:17 Changed 7 years ago by Martyn Gigg

Fix passing weak_ptrs back to C++ from Python layer.

Refs #8996

Changeset: 096a518959b38f1b0fff7ba2a0c368e66acd4666

comment:18 Changed 7 years ago by Martyn Gigg

Rename RegisterDataItemInterface to DataItemInterface.

Refs #8996

Changeset: 52351aeac24d6586e154405581ef0192c3d76878

comment:19 Changed 7 years ago by Martyn Gigg

Remove a macro in favour of direct WorkspacePropertyExporter usage.

Refs #8996

Changeset: 77a48d6e6bbd61073ba6e8b3d880b65c7e619913

comment:20 Changed 7 years ago by Martyn Gigg

Remove another macro in favour of a templated static class

Refs #8996

Changeset: ad5b4f43978a0f21160230f38138c92c2089f023

comment:21 Changed 7 years ago by Martyn Gigg

Remove another macro in a header in favour of template class

Refs #8996

Changeset: eabcca5f7d7d218cbd27a114b866f0f128a8591c

comment:22 Changed 7 years ago by Martyn Gigg

Remove a pointless namespace declaration.

Refs #8996

Changeset: 58c9ccede9184b3f855c8180e1d35a7fd0fe8fd1

comment:23 Changed 7 years ago by Martyn Gigg

An ugly cast for interfaces that don't inherit from DataItem.

Refs #8996

Changeset: 2da84ba1fbf8ef799272c6a7f573f6352410fa1d

comment:24 Changed 7 years ago by Martyn Gigg

Change header reference in CMakeLists file.

Refs #8996

Changeset: 70e30d30ae332e1e7fce24b69d94116c6ba64b21

comment:25 Changed 7 years ago by Martyn Gigg

Kill a compiler warning about unused exception variable.

Refs #8996

Changeset: 5a99e082157b0d7c4fb4c421397659d324aa8de6

comment:26 Changed 7 years ago by Martyn Gigg

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

Branch: feature/8996_python_dataitem_casting

Tester: These changes are required to be able to correctly cast to IMaskWorkspace when it is exported in #8943. There should be no visible changes in usage from Python and the only 3 tests that were changed were altered to remove redundant lines. Obviously the unit & system tests should be passing in all places without any other changes.

Several places in the code were tidied up along the way to reduce the number of macros in headers and to make the TypeRegistry & DowncastRegistry more consistently designed.

comment:27 Changed 7 years ago by Peter Peterson

  • Status changed from verify to verifying
  • Tester set to Peter Peterson

comment:28 Changed 7 years ago by Peter Peterson

  • Status changed from verifying to verify
  • Tester Peter Peterson deleted

I'm having a hard time verifying this one as I merged this into master, then the masking one, and still no masking properties on a MaskWorkspace.

comment:29 Changed 7 years ago by Martyn Gigg

So I've completely restructed the casting stuff so the masking one shouldn't even merge cleanly when this goes in.

Getting IMaskWorkspace will now require a few extra lines in the IMaskWorkspace.cpp export file

using Mantid::PythonInterface
DataItemInterface<IMaskWorkspace>()
  // map IDs to this interface
  .castFromID("MaskWorkspace")
;

along with removing the id "MaskWorkspace" from the list in the PythonInterface/mantid/api/src/Exports/MatrixWorkspace.cpp at the bottom, remembering to update the NUM_IDS counter.

comment:30 Changed 7 years ago by Martyn Gigg

  • Blocking 8304 added

comment:31 Changed 7 years ago by Anders Markvardsen

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

comment:32 Changed 7 years ago by Anders Markvardsen

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

I am going to struggle fitting this one in today. If this here by Monday will look at it agains

comment:33 Changed 7 years ago by Alex Buts

  • Status changed from verify to verifying
  • Tester set to Alex Buts

comment:34 Changed 7 years ago by Alex Buts

  • Status changed from verifying to verify
  • Tester Alex Buts deleted

Its nice ticket which I would be interested to learn but it needs some time to spend on and unfortunately more urgent issue come, which I have to deal with.

comment:35 Changed 7 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:36 Changed 7 years ago by Owen Arnold

  • Keywords CORE added

comment:37 Changed 7 years ago by Owen Arnold

Everything looks to be in order. Simple solution. Lot of additional changes required to make it happen.

comment:38 Changed 7 years ago by Owen Arnold

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/feature/8996_python_dataitem_casting'

Full changeset: 05fd06fca4923e9aca9d68fefb91bab29948bd32

comment:39 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 9839

Note: See TracTickets for help on using tickets.