Update for subsample masking API #167
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
Closed
mmuckley
force-pushed
the
mmuckley/masking_update
branch
2 times, most recently
from
8451f29
to
18cb8bb
Compare
mmuckley
force-pushed
the
mmuckley/masking_update
branch
from
18cb8bb
to
12cf3a5
Compare
luisenp
reviewed
Sep 15, 2021
| @@ -252,6 +253,29 @@ def normalize_instance( | |||
| return normalize(data, mean, std, eps), mean, std | |||
|
|
|||
|
|
|||
| class UnetSample(NamedTuple): | |||
There was a problem hiding this comment.
Mostly out of curiosity, any reason to prefer this over a dataclass?
There was a problem hiding this comment.
To make it more like a tuple! Immutability is great.
mmuckley
force-pushed
the
mmuckley/masking_update
branch
from
ef38421
to
56d49c1
Compare
|
I'm going to go ahead and merge this - ran some tests on the knee data and it seems SSIM is the same throughout the training process. I'll do a full validation with leaderboard submission prior to the next package release to PyPI. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This update implements a major overhaul of the masking API for the repository. The primary reason for doing this was to implement the magic mask in the core repository. Already, we had repeated code in several sections, and with the new mask this would be duplicated again and be bad for maintenance. With the new code structure, everything is subclassed from a base mask function in a modular way, so for the future we'll hopefully only need to change the base class to make updates.
We also implemented a few other things that came from the internal fastMRI repository (most of which has already been exposed via
banding_removal). These include returning the number of low frequency lines (for sensitivity map estimation) and segmenting out the equispaced mask from the pseudo-equispaced mask mentioned in Issue #54.Note: for the old equispaced mask behavior, you now need to use
EquispacedMaskFraction.EquispacedMasknow implements exactly an equispaced mask, as its title suggests. The VarNet and U-Net examples should be updated accordingly with this PR.Lastly, this PR updates the test environment to bring all dependencies up to date for September 2021 for unit testing. It supersedes #166, as it seems the
mypyupdate required changes to several parts of the code, so I thought it was easier to include here.CC @adefazio @Timsey