Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Make dict_from_transform more consistent with other dict representations #4635

Merged

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented May 1, 2024

Encapsulate the "transformParameterization", "parametersValueType",
"inputDimension", "outputDimension", which are static, into a
"transformType" member, similar to "imageType", "meshType" for Images,
Meshes. "transformParameterization" is the string name of the ITK transform
class, less the trailing "Transform".

Example serialization:

{
    'transformType': {'transformParameterization': 'Translation', 'parametersValueType': 'float64', 'inputDimension': 3, 'outputDimension': 3},
    'name': '',
    'inputSpaceName': '',
    'outputSpaceName': '',
    'parameters': array([3., 2., 8.]),
    'fixedParameters': array([], dtype=float64),
    'numberOfParameters': 3,
    'numberOfFixedParameters': 0
}
[
    {
        'transformType': {'transformParameterization': 'Translation', 'parametersValueType': 'float64', 'inputDimension': 3, 'outputDimension': 3},
        'name': '',
        'inputSpaceName': '',
        'outputSpaceName': '',
        'parameters': array([3., 2., 8.]),
        'fixedParameters': array([], dtype=float64),
        'numberOfParameters': 3,
        'numberOfFixedParameters': 0
    },
    {
        'transformType': {'transformParameterization': 'Affine', 'parametersValueType': 'float64', 'inputDimension': 3, 'outputDimension': 3},
        'name': '',
        'inputSpaceName': '',
        'outputSpaceName': '',
        'parameters': array([  0.6563149 ,   0.58065837,  -0.48175367,  -0.74079868,
         0.37486398,  -0.55739959,  -0.14306664,   0.72271215,
         0.67617978, -66.        ,  69.        ,  32.        ]),
        'fixedParameters': array([0., 0., 0.]),
        'numberOfParameters': 12,
        'numberOfFixedParameters': 3
    }
]

Remove unused numpy import in setstate.

Support both a list of transforms and a single transform in
dict_from_transform.

Add more smoke tests in extras.py.

@thewtex thewtex requested a review from PranjalSahu May 1, 2024 21:34
@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels May 1, 2024
@PranjalSahu
Copy link
Contributor

PranjalSahu commented May 1, 2024

Is it possible to share a sample json output (dict representation for a transform) for a single and composite transform?
That will make these changes very clear.

Something similar to #3470

@PranjalSahu
Copy link
Contributor

I only have a question regarding the need to support a list of transforms in the dict_from_transform method. Since it already handles multiple transform use cases under the CompositeTransform Object.

Also supporting a list of transforms will make this different compared to Image and Mesh methods which only work on one Image/Mesh Object.

@thewtex
Copy link
Member Author

thewtex commented May 2, 2024

Is it possible to share a sample json output (dict representation for a transform) for a single and composite transform?
That will make these changes very clear.

Yes, added to the PR description and commit.

@thewtex
Copy link
Member Author

thewtex commented May 2, 2024

I only have a question regarding the need to support a list of transforms in the dict_from_transform method. Since it already handles multiple transform use cases under the CompositeTransform Object.

Also supporting a list of transforms will make this different compared to Image and Mesh methods which only work on one Image/Mesh Object.

Yes, it is a bit different from Image/Mesh since we have a chain of transforms. Support for this use case came up in the test for working with the result of itk.transformread. We could have:

  • A single transform
  • A list of transforms that is generated by itk.transformread
  • Transforms that result from flattening CompositeTransform's

I added docstrings to help clarify.

…ntations

Encapsulate the "transformParameterization", "parametersValueType",
"inputDimension", "outputDimension", which are static, into a
"transformType" member, similar to "imageType", "meshType" for Images,
Meshes. "transformParameterization" is the string name of the ITK transform
class, less the trailing "Transform".

Example serializations:

Translation:

```py
{
    'transformType': {'transformParameterization': 'Translation', 'parametersValueType': 'float64', 'inputDimension': 3, 'outputDimension': 3},
    'name': '',
    'inputSpaceName': '',
    'outputSpaceName': '',
    'parameters': array([3., 2., 8.]),
    'fixedParameters': array([], dtype=float64),
    'numberOfParameters': 3,
    'numberOfFixedParameters': 0
}
```

Composite:

```py
[
    {
        'transformType': {'transformParameterization': 'Translation', 'parametersValueType': 'float64', 'inputDimension': 3, 'outputDimension': 3},
        'name': '',
        'inputSpaceName': '',
        'outputSpaceName': '',
        'parameters': array([3., 2., 8.]),
        'fixedParameters': array([], dtype=float64),
        'numberOfParameters': 3,
        'numberOfFixedParameters': 0
    },
    {
        'transformType': {'transformParameterization': 'Affine', 'parametersValueType': 'float64', 'inputDimension': 3, 'outputDimension': 3},
        'name': '',
        'inputSpaceName': '',
        'outputSpaceName': '',
        'parameters': array([  0.6563149 ,   0.58065837,  -0.48175367,  -0.74079868,
         0.37486398,  -0.55739959,  -0.14306664,   0.72271215,
         0.67617978, -66.        ,  69.        ,  32.        ]),
        'fixedParameters': array([0., 0., 0.]),
        'numberOfParameters': 12,
        'numberOfFixedParameters': 3
    }
]
```

Remove unused numpy import in __setstate__.

Support both a list of transforms and a single transform in
dict_from_transform.

Add more smoke tests in extras.py.
@thewtex thewtex added this to the ITK 5.4.0 milestone May 2, 2024
@PranjalSahu
Copy link
Contributor

LGTM

Copy link
Contributor

@PranjalSahu PranjalSahu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thewtex thewtex merged commit afc4879 into InsightSoftwareConsortium:master May 3, 2024
14 checks passed
@thewtex thewtex deleted the transform-dict-repr branch May 3, 2024 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:Python wrapping Python bindings for a class type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants