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

Revert invertibility of RescaleIntensity #1120

Merged

Conversation

fepegar
Copy link
Owner

@fepegar fepegar commented Nov 1, 2023

Implement changes proposed in #1116.

@nicoloesch @romainVala @mueller-franzes

@fepegar fepegar mentioned this pull request Nov 1, 2023
8 tasks
Copy link
Contributor

@nicoloesch nicoloesch left a comment

Choose a reason for hiding this comment

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

The transform is in the current state (with the proposed changes) still invertible as it has the attribute self.revert_transform (see transform.py in L.403 with the method is_invertible). As such, I would suggest to either overwrite is_invertible as proposed in #1116 or removing the entire code part for invert_transform. I would suggest the former as we do not loose any functionality nor do we make it more complicated - we just need to update the docstring (which has already been done in #1116 in my stashes but I am awaiting the response of all people of the issue). If that is choosen, the test needs to be adapted and needs to have two checks - one that it fails if in_min_max is not provided and one where it actually is invertible and that the inverse is the same again.

@fepegar
Copy link
Owner Author

fepegar commented Nov 1, 2023

You're right, this still needs some work. I'll let you choose on top of these changes or your stash.

I didn't get this bit though:

the test needs to be adapted and needs to have two checks - one that it fails if in_min_max is not provided and one where it actually is invertible and that the inverse is the same again.

Shouldn't we get rid of those tests?

@romainVala
Copy link
Contributor

thanks @nicoloesch ! sound good to me

@fepegar
Copy link
Owner Author

fepegar commented Nov 4, 2023

Given the importance of the bug, I'm going to go ahead and merge this PRs. The code is still in the repo's history if we ever needed back. Thank you all for your contribution!

@fepegar fepegar merged commit 6dba7be into 1115-fix-rescale-intensity Nov 4, 2023
2 checks passed
@fepegar fepegar deleted the 1115-bis-revert-rescale-intensity branch November 4, 2023 20:16
fepegar added a commit that referenced this pull request Nov 4, 2023
* Add failing test

* Revert invertibility of `RescaleIntensity` (#1120)

* Make RescaleIntensity non-invertible again

* Disable support to invert RescaleIntensity
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.

3 participants