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

Fix error with invertible custom transforms, also keep include/exclude lists for inverse transforms. #398

Merged
merged 4 commits into from
Dec 27, 2020

Conversation

efirdc
Copy link
Contributor

@efirdc efirdc commented Dec 27, 2020

Description
This fixes two bugs that I found when making a custom invertible Transform.

First is that Subject.history would not find any transforms defined outside torchio. To fix this I added a get_subclasses(target_class) function to utils.py.

Other thing I noticed was that Transform._get_reproducing_arguments() did not keep the include/exclude lists. So I added them there.

Checklist

  • I have read the CONTRIBUTING docs and have a developer setup (especially important are pre-commitand pytest)
  • Non-breaking change (would not break existing functionality)
  • Breaking change (would cause existing functionality to change)
  • Tests added or modified to cover the changes
  • Integration tests passed locally by running pytest
  • In-line docstrings updated
  • Documentation updated, tested running make html inside the docs/ folder
  • This pull request is ready to be reviewed
  • If the PR is ready and there are multiple commits, I have attempted to squash them and force-push

@codecov
Copy link

codecov bot commented Dec 27, 2020

Codecov Report

Merging #398 (2fe0433) into master (c9d9b42) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   92.41%   92.42%   +0.01%     
==========================================
  Files         117      117              
  Lines        5935     5943       +8     
==========================================
+ Hits         5485     5493       +8     
  Misses        450      450              
Impacted Files Coverage Δ
torchio/data/subject.py 94.53% <100.00%> (+0.08%) ⬆️
torchio/transforms/transform.py 94.01% <100.00%> (+0.07%) ⬆️
torchio/utils.py 89.25% <100.00%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9d9b42...2fe0433. Read the comment docs.

@fepegar
Copy link
Owner

fepegar commented Dec 27, 2020

Nice. The invertibility feature is very young and I'm sure there is still a lot of room for improvement. Many thanks for your contribution, @efirdc!

@fepegar fepegar merged commit 13aa314 into fepegar:master Dec 27, 2020
@fepegar
Copy link
Owner

fepegar commented Dec 27, 2020

@all-contributors please add @efirdc for code

@allcontributors
Copy link
Contributor

@fepegar

I've put up a pull request to add @efirdc! 🎉

@romainVala
Copy link
Contributor

Hello
just out of curiosity, @efirdc do you use transform outside torchio (which one, how it is compatible with torchio) ?

@efirdc
Copy link
Contributor Author

efirdc commented Jan 5, 2021

Hi @romainVala,

At the time it was the label transformations, but they are included in torchio now. At the moment I'm not using any custom transformations.

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