Ticket #8996 (closed: fixed)
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: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: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: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