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 HistogramStandardization example #1022

Merged
merged 5 commits into from
Mar 4, 2023
Merged

Fix HistogramStandardization example #1022

merged 5 commits into from
Mar 4, 2023

Conversation

vedal
Copy link
Contributor

@vedal vedal commented Feb 7, 2023

Fixes #{issue_number}.

Description
Currently the example isn't loading the landmarks from path. This PR fixes that.

Checklist

  • [v] I have read the CONTRIBUTING docs and have a developer setup (especially important are pre-commitand pytest)
  • [v] 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
  • [v] In-line docstrings updated
  • Documentation updated, tested running make html inside the docs/ folder
  • [v] This pull request is ready to be reviewed
  • If the PR is ready and there are multiple commits, I have squashed them and force-pushed

@fepegar
Copy link
Owner

fepegar commented Feb 11, 2023

Hi, @vedal. Please read the contributing guide as requested in the PR template.

According to the documentation, this transform supports paths to NumPy arrays, so this change is not needed.

@fepegar fepegar closed this Feb 11, 2023
@vedal
Copy link
Contributor Author

vedal commented Feb 11, 2023

Hi @fepegar, thanks for reviewing this. I think particularly the change to add a torch.load makes the example more readable, since the variable name "t1_landmarks" suggests its not a path (such as "t1_landmarks_path" used previously).
As such, the change might not be necessary but at least increase readiblity.

Also, its unusual to store .npy with torch.save, as done if running the example once.

@fepegar fepegar reopened this Feb 11, 2023
@fepegar
Copy link
Owner

fepegar commented Feb 11, 2023

Thanks for clarifying, that makes sense. I'll take a look later.

@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.59%. Comparing base (aa4d609) to head (2af647f).
Report is 102 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1022      +/-   ##
==========================================
+ Coverage   86.47%   86.59%   +0.11%     
==========================================
  Files          91       91              
  Lines        5774     5787      +13     
==========================================
+ Hits         4993     5011      +18     
+ Misses        781      776       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fepegar
Copy link
Owner

fepegar commented Feb 11, 2023

I think the idea is that the list of landmarks is saved an an npy file, and pth would be used for a pickled dict. So I think the change I would do is replace torch.save(t1_landmarks, t1_landmarks_path) with np.save(t1_landmarks_path, t1_landmarks). What do you think?

@fepegar fepegar marked this pull request as draft February 28, 2023 20:38
@fepegar fepegar marked this pull request as ready for review March 4, 2023 18:10
@fepegar fepegar changed the title load landmarks if path exists Fix HistogramStandardization example Mar 4, 2023
@fepegar fepegar merged commit 863a038 into fepegar:main Mar 4, 2023
@fepegar
Copy link
Owner

fepegar commented Mar 4, 2023

@all-contributors please add @vedal for doc

@allcontributors
Copy link
Contributor

@fepegar

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

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