Skip to content

Further options for the transformations#14

Merged
dzenanz merged 3 commits intoInsightSoftwareConsortium:masterfrom
romangrothausmann:scaleWithR
Feb 21, 2020
Merged

Further options for the transformations#14
dzenanz merged 3 commits intoInsightSoftwareConsortium:masterfrom
romangrothausmann:scaleWithR

Conversation

@romangrothausmann
Copy link
Copy Markdown
Member

@romangrothausmann romangrothausmann commented Feb 19, 2020

This PR adds the option to scale the transform with the radius. This allows to use it with the itkResampleImageFilter to realize a transformation from cart. coords to polar/cylindrical coords (unfold, similar to http://hdl.handle.net/10380/3605) such that the arc-length/surface area is preserved.
It also adds the option to return NaN for values outside [-pi,pi], such that itkResampleImageFilter uses the DefaultPixelValue instead of repeating the pattern periodically, and the option to specify an offset for the angle allowing to adjust the center of the output.

@romangrothausmann
Copy link
Copy Markdown
Member Author

Why does 67b6b68 (similar to itkImageFunctionTest.cxx) lead to:

In file included from /opt/itk/include/ITK-5.1/itkPolarToCartesianTransform.h:188:0,
                 from /code/unfold.cxx:7:
/opt/itk/include/ITK-5.1/itkPolarToCartesianTransform.hxx: In member function 'itk::PolarToCartesianTransform<TParametersValueType, NDimensions>::OutputPointType itk::PolarToCartesianTransform<TParametersValueType, NDimensions>::TransformPoint(const InputPointType&) const':
/opt/itk/include/ITK-5.1/itkPolarToCartesianTransform.hxx:65:74: error: type/value mismatch at argument 1 in template parameter list for 'template<class T> class itk::NumericTraits'
       using PointNumericTraits = NumericTraits<OutputPointType::ValueType>;
                                                                          ^
/opt/itk/include/ITK-5.1/itkPolarToCartesianTransform.hxx:65:74: note:   expected a type, got 'itk::PolarToCartesianTransform<TParametersValueType, NDimensions>::OutputPointType:: ValueType'

while 5759bec works?

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 19, 2020

You might need a typename, e.g. turn using PointNumericTraits = NumericTraits<OutputPointType::ValueType>; into using PointNumericTraits = NumericTraits<typename OutputPointType::ValueType>;.

@romangrothausmann romangrothausmann force-pushed the scaleWithR branch 2 times, most recently from 1cd9c24 to ce8459d Compare February 21, 2020 10:22
@romangrothausmann
Copy link
Copy Markdown
Member Author

Many thanks @dzenanz for that hint, works well now.

@romangrothausmann romangrothausmann force-pushed the scaleWithR branch 3 times, most recently from a3b936b to 16f7334 Compare February 21, 2020 11:08
@romangrothausmann
Copy link
Copy Markdown
Member Author

Many thanks @dzenanz for the approval, it was not finished then but is now, so if a6ee109 is OK too, it's ready for merge.

@romangrothausmann romangrothausmann changed the title WIP: optional scale transform with the radius Further options for the transformations Feb 21, 2020
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 21, 2020

NumFOCUS copyright is unrelated. Merging.

@dzenanz dzenanz merged commit 14dd8bd into InsightSoftwareConsortium:master Feb 21, 2020
@romangrothausmann
Copy link
Copy Markdown
Member Author

Thanks @dzenanz for merging. Was it intended that it was a fast-forward and not a merge commit? This way the PR number (and its discussion and tests) is not recorded in the history.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 24, 2020

It was intentional. I prefer rebase-and-merge because it does not produce merge commits which are almost as numerous as content commits. I did not realize this makes it hard to work back from which PR they came. Your comment made me realize that merge is preferable when there is significant discussion in a PR, so thanks.

@romangrothausmann
Copy link
Copy Markdown
Member Author

Yes, I used to wonder what --no-ff was good for but learned then that it is quite good to group and summarize the content of the "sub-commits" of the longer parent side.
If you have the ability to push to master you can still create a merge commit manually (before the next PR is merged):

git checkout 14dd8bdb364e3ddba76739d8dfbb8247c91d3377 -b scaleWithR
git checkout master
git reset --hard d9846fb13348ca7864c438a086b8d7c9cd34be27
git merge --no-ff scaleWithR -m 'Merge pull request #14 from romangrothausmann/scaleWithR'
gitk --all  # to check if all worked out as expected
git push

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 25, 2020

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants