-
Notifications
You must be signed in to change notification settings - Fork 9
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
Refactor Portilla-Simoncelli model #225
Conversation
and corrects some docstrings
…noptic into ps_refactor
move that out of the bowels of PortillaSimoncelli, because it might be helpful
beause it's getting automatically added by something
This refactors PS to: - remove all unnecessary attributes (representation, etc) - make forward() much more straightforward, calling transparently-named methods that return the thing they say they do
- removes old autocorr, nothing uses it - adds new autocorrelation, the way needed for portilla simoncelli - puts expand and shrink in signal.py
this makes it possible to vmap it
… into ps_refactor
…enoptic into ps_refactor
refactor to use the non-downsampled pyramid. probably won't stick with this, because it's ~2x slower on the cpu, and fairly different values (rtol=1e-1, atol=1e-4 ish)
it's actually more efficient, as long as we can't use vmap (which we can't)
third version of this, but I think this is the way. still use the downsampling pyramid, but now make lists of tensors (one per scale) to use. gets us an intermediate version between the two, while stlil passing all the tests
…enoptic into ps_refactor
adds longer discussion in notebook about what metamers mean in the PS texture context and why they're useful. also renames vector -> tensor, because that's clearer
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.
I like the changes. Looks good to me
Returns | ||
------- | ||
representation_vector: | ||
3d tensor of shape (B,C,S) containing the measured texture |
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.
I like it more now
SCALES_TYPE = Union[ | ||
int, Literal["pixel_statistics", "residual_lowpass", "residual_highpass"] | ||
] |
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.
I like it now, it is consistent!
reconstructed_images.append(recon + reconstructed_images[-1]) | ||
# now downsample as necessary, so that these end up the same size as | ||
# their corresponding coefficients. | ||
reconstructed_images[:-1] = [signal.shrink(r, 2**(self.n_scales-i)) * 4**(self.n_scales-i) |
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.
I think either choices work, as long as you explain what is happening. If you choose to keep the pixel value the same I would add a comment before this line (when you multiplying by 4**(n_scales -1) ) and a note in the shrink function saying, "this function maintains the pixel values fixed; the power however will change. If you need to preserve the power, multiply by...".
If you chose the for having the power as an invariant, I would also make it clear in the docstrings of the shrink/expand: something like, "this function down-samples the image and scales it to preserve the power..."
|
||
def plot_representation( |
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.
yep, I agree with the separate PR
0, (2 * self.n_orientations, max(2 * self.n_orientations, 5), self.n_scales) | ||
) | ||
n_filled += nn | ||
def convert_to_dict(self, representation_vector: Tensor) -> OrderedDict: |
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.
I see, that makes sense to me; If there is no way to make it general, I agree it's not worth the effort
@@ -1,55 +1,58 @@ | |||
import torch |
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.
that works
with finished discussion of magmeans, more mixture discussion
This is ready to go now, after I push the merge with main. I changed the description of magnitude means at the very end. |
This pull request completely refactors the Portilla-Simoncelli texture model. As part of this, following changes to
po.simul.PortillaSimoncelli
:model.to(torch.float64)
in order to accept double-precision inputs -- will do so automatically.forward
use_true_correlations=False
. We now only support using the true correlations, because the only reason someone would ever set it to False was to check against matlab. I still test against the matlab, but this now requires a bit more work, which now all lives in tests.Other changes:
tools/
or added:center_crop
(no longer requires torchvision),expand
(upsample an image using Fourier transform),shrink
(downsample an image using Fourier transform),modulate_phase
(modulate the phase of a complex signal, e.g., double the phase of a steerable pyramid coefficient for correlating it with another scale),autocorrelation
(replaces and slightly generalizes existingautocorr
function).Still to do:
Notes:
Closes: #199, #142