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.

Sign up for GitHub

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

Jump to bottom

Deprecate ...CoordRepType and replace them with ...CoordinateType #5006

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Deprecate ...CoordRepType and replace them with ...CoordinateType #5006

merged 4 commits into from
Dec 4, 2024

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Dec 3, 2024

Triggered by a comment from Hans (@hjmjohnson) at #4999 (review)

@N-Dekker
ENH: Add ...CoordinateType aliases as alternative to ...CoordRepType
42f1565 
Follow-up to pull request InsightSoftwareConsortium#4997
commit 5ea1a9a
"ENH: Add nested CoordinateType aliases as alternative to CoordRepType"
@N-Dekker
STYLE: Replace ...CoordRepType with ...CoordinateType in tests
f7a9236 
Follow-up to pull request InsightSoftwareConsortium#4997
commit 324eaf1
"STYLE: Replace CoordRepType with CoordinateType in tests"
@github-actions github-actions bot added 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 area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module labels Dec 3, 2024
@dzenanz
Copy link
Member

dzenanz commented Dec 3, 2024

Good job working on this so far. Is there any chance you can update the remote modules, at least the more commonly used ones, which are tested nightly? E.g. Montage now has a lot of warnings.

@N-Dekker
STYLE: Replace ...CoordRepType with ...CoordinateType within ITK
4187d82 
Follow-up to pull request InsightSoftwareConsortium#4997
commit 15d189f
"STYLE: Replace CoordRepType with CoordinateType in ITK implementation"
@dzenanz
Copy link
Member

dzenanz commented Dec 3, 2024

Also, in order to build it against ITK 5.x, do we need to put some ifdefs there? This probably applies to other user code - ITK5/6 ifdef, or turn on legacy. Should we document this in the migration guide?

@N-Dekker
ENH: FUTURE REMOVE ...CoordRepType in favor of ...CoordinateType
80dad29 
Follow-up to pull request InsightSoftwareConsortium#4997
commit e667fa3
"ENH: Deprecate (FUTURE REMOVE) CoordRepType in favor of CoordinateType"
@N-Dekker N-Dekker force-pushed the Add-prefixed-CoordinateType branch from 9efee43 to 80dad29 Compare December 3, 2024 17:00
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Dec 3, 2024

Thanks @dzenanz

First of all, none of these changes should trigger a compile error on any user code, except when FUTURE_LEGACY_REMOVE is ON. Moreover, none of the changes should produce a warning on any user code, as long as the user enabled legacy support (LEGACY_REMOVE is OFF). (But only after PR #4999 was merged)

Was Montage built with or without legacy support? If it was built with legacy support (LEGACY_REMOVE is OFF), the warnings should no longer appear after PR #4999.

Indeed, a note should be added to the migration guide. Thanks for reminding me 😃 I can make a PR to do so, right after this one (#5006) is merged.

@dzenanz
Copy link
Member

dzenanz commented Dec 3, 2024

Yes, legacy was turned off. You can see the full configuration here:
https://open.cdash.org/builds/10063093/notes

@N-Dekker N-Dekker marked this pull request as ready for review December 3, 2024 19:34
@hjmjohnson hjmjohnson merged commit a68b8f6 into InsightSoftwareConsortium:master Dec 4, 2024
17 checks passed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 4, 2024
@N-Dekker
ENH: Add ...CoordinateType aliases as alternative to ...CoordRepType
428bc74 
Redid pull request InsightSoftwareConsortium#5006
commit fbe9b04 on release-5.4.
hjmjohnson pushed a commit that referenced this pull request Dec 5, 2024
@N-Dekker @hjmjohnson
ENH: Add ...CoordinateType aliases as alternative to ...CoordRepType
be27beb 
Redid pull request #5006
commit fbe9b04 on release-5.4.
N-Dekker added a commit to N-Dekker/ITKMontage that referenced this pull request Dec 6, 2024
@N-Dekker
COMP: Replace CoordRepType with CoordinateType
adb8be5 
Addressed the warnings at https://open.cdash.org/viewBuildError.php?type=1&buildid=10063093
reported by Dženan Zukić at InsightSoftwareConsortium/ITK#5006 (comment)

Following ITK pull request InsightSoftwareConsortium/ITK#4997
commit InsightSoftwareConsortium/ITK@e667fa3
@N-Dekker N-Dekker mentioned this pull request Dec 6, 2024
Draft
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Dec 6, 2024

Should we document this in the migration guide?

@dzenanz Proposed note:

For the sake of code readibility, a new `CoordinateType` alias is added for
each nested `CoordRepType` alias. The old `CoordRepType` aliases will still be
available with ITK 6.0, but it is recommended to use `CoordinateType` instead.
The `CoordRepType` aliases will be removed when `ITK_FUTURE_LEGACY_REMOVE` is
enabled. Similarly, `InputCoordinateType`, `OutputCoordinateType`, and
`ImagePointCoordinateType` also replace the corresponding `CoordRepType`
aliases.

(I'm not yet able to make it a pull request, because my clang-format isn't yet properly installed/configured.)

@dzenanz
Copy link
Member

dzenanz commented Dec 6, 2024

From this commit: 0b1fabd, get Windows binary from this link.

@N-Dekker N-Dekker mentioned this pull request Dec 16, 2024
Merged
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 16, 2024
@N-Dekker
DOC: Note migration guide replacing CoordinateType with CoordRepType
c853a05 
Added note about `CoordRepType` being replaced with `CoordinateType`.

Follow-up to:
- pull request InsightSoftwareConsortium#5006
commit a68b8f6
"ENH: FUTURE REMOVE `...CoordRepType` in favor of `...CoordinateType`"
- pull request InsightSoftwareConsortium#4997
commit e667fa3
"ENH: Deprecate (FUTURE REMOVE) CoordRepType in favor of CoordinateType"
hjmjohnson pushed a commit that referenced this pull request Dec 16, 2024
@N-Dekker @hjmjohnson
DOC: Note migration guide replacing CoordinateType with CoordRepType
44e7133 
Added note about `CoordRepType` being replaced with `CoordinateType`.

Follow-up to:
- pull request #5006
commit a68b8f6
"ENH: FUTURE REMOVE `...CoordRepType` in favor of `...CoordinateType`"
- pull request #4997
commit e667fa3
"ENH: Deprecate (FUTURE REMOVE) CoordRepType in favor of CoordinateType"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz approved these changes

@hjmjohnson hjmjohnson hjmjohnson approved these changes

Assignees
No one assigned
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@N-Dekker @dzenanz @hjmjohnson