-
Notifications
You must be signed in to change notification settings - Fork 26
Validate Downsample #1252
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
Validate Downsample #1252
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1252 +/- ##
========================================
Coverage 90.61% 90.61%
========================================
Files 132 132
Lines 14143 14153 +10
========================================
+ Hits 12815 12825 +10
Misses 1328 1328 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Attaching the notebooks that validate Image and Volume downsample (with |
65c7c04 to
927ee68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just one thing.
| ims_ds_legacy = ims.downsample(res_ds, legacy=True) | ||
|
|
||
| # ASPIRE-Python downsample with centering adjustments for odd resolution images. | ||
| shifts = 0.5 * np.ones((n_img, 2), dtype=dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we replace this with the formula that we found for the shifts? I'm worried that this is only half-pixel in certain special cases like this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My notes from our meeting are a little incomplete, but what I have working locally for each case is this.
even -> even: no shift needed
even -> odd: shift by 1/2
odd -> odd: shift by 1/2 - res_ds/(2 * res_og)
odd -> even: shift by -res_ds/(2 * res_og)
I'm not getting allclose on the odd -> even case, so maybe I have something wrong here. For what it's worth, the original test works for different downsample ratios since the shifting occurs before and after downsampling depending on where the odd resolution is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But shouldn't the amount of shifting required differ depending on the downsampling ratio (per the formulas you give above)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Edit Above: I had a typo in the formulas above... was missing a factor of 2 in the denominators.]
Yes, but the 1/2 pixel shift (on line 205) which happens prior to downsampling is converted in the downsampled image into a shift of res_ds/(2 * res_og) pixels by the act of downsampling. So the ratio ends up being accounted for. I believe this is correct and have confirmed it for several different downsample ratios, but it's possible I'm misunderstanding something.
I suspect that I am not getting allclose for the last case due to some difference in our Image.shift method on even vs odd images.
Validates downsample against Matlab to address #1244
This PR creates a
legacyflag forImage.downsample/Volume.downsamplethat reproduces the Matlab downsample output up tonp.allclosetolerances.I will upload notebooks, with matlab invocations documented, that demonstrate these results. I still need add a legacy flag test and check whether some desired properties hold.