Tools and examples organization #19
Conversation
- fftshift works beyond 2d, renamed everywhere - kurtosis and skew in new stats file - rename visualization to display - TODO organize metamer utils
- typo in basic stimuli - Added simple model: Steerable followed by gain control (can be real or complex)
plenoptic/simulate/canonical_computations/steerable_pyramid_freq.py
Outdated
Show resolved
Hide resolved
because we very well might have more than 10 of them
plenoptic/simulate/canonical_computations/steerable_pyramid_freq.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I made several comments that I think need to be addressed, but they're minor:
-
add docstrings. couple functions that are new, some that we just haven't gotten around to writing them for yet
-
there are several TODOs scattered throughout this that you added. Are they things we want done before merging this PR or that we can wait on? If the latter, let's open issues for them so we don't forget
-
do we need to add an implementation for AlexNet? Rather than just using the pytorch one
and I added some comments either answering or asking for clarification
should have a max of one empty line within functions (two is used to separate functions or classes)
…king steering code for torch batch coeffs
- further clean-up
Furhter nice to have features:
|
| @@ -6,10 +6,10 @@ | |||
|
|
|||
| class Steerable_GainControl(nn.Module): | |||
| """steerable pyramid followed by local gain control""" | |||
| def __init__(self, shape, cmplx=False): | |||
| def __init__(self, shape, is_complex=False): | |||
| super().__init__() | |||
There was a problem hiding this comment.
Is there a reason why order and height shouldn't also be args for this class?
There was a problem hiding this comment.
Good point, some of these changes have been made, and I propose to move this to a model_zoo branch
| @@ -32,7 +32,7 @@ def torch_complex_to_numpy(x): | |||
|
|
|||
| def make_basic_stimuli(size=256, requires_grad=True): | |||
|
|
|||
| # TODO assert size is a power of 2 | |||
| assert size in [32, 64, 128, 256, 512], 'size not supported' | |||
| impulse = np.zeros((size, size)) | |||
There was a problem hiding this comment.
Are all powers of 2 supported, or just these?
There was a problem hiding this comment.
Right now, this is what we support, making it fully general is not a priority to me.
My use case for this function is to get a set of simple stimuli to develop and debug models with
There was a problem hiding this comment.
Okay, can we get a small docstring that includes that info?
plenoptic/tools/data.py
Outdated
| @@ -61,7 +61,7 @@ def make_basic_stimuli(size=256, requires_grad=True): | |||
| checkerboard = plt.imread(op.join(DATA_PATH, 'checkerboard.pgm')).astype(float) | |||
| # adjusting form 256 to desired size | |||
| l = int(np.log2(256 // size)) | |||
| # TODO for larger size use upConv | |||
| # for larger size use upConv | |||
There was a problem hiding this comment.
I don't understand this comment (because I don't see upConv anywhere). What's it mean?
There was a problem hiding this comment.
this is if we wanted to work with large images, we could start with a 256x256 and upsample_convolve it.
this last function lives in tools/conv.py, which is meant to be a torch port and generalization of pyrtools equivalents.
There was a problem hiding this comment.
Okay. Can we remove this line and, if you think it's important, open an issue?
plenoptic/simulate/canonical_computations/steerable_pyramid_freq.py
Outdated
Show resolved
Hide resolved
… steering code to be cleaner
merge master fix for new pytorch version
plenoptic/simulate/canonical_computations/steerable_pyramid_freq.py
Outdated
Show resolved
Hide resolved
…ng tight_frame pyramid, minor bug fixes and refactoring, additional pyramid tests, added multidimensional min/max for torch tensors, updated pyramid tutorial
these have been deleted
image should be float32, not float64
| f2 = gaussian(kernel_size, covariance=1) - f1 | ||
| self.conv1.weight.data[0,0] = nn.Parameter(torch.tensor(f1, dtype=torch.float32)) | ||
| self.conv1.weight.data[1,0] = nn.Parameter(torch.tensor(f2, dtype=torch.float32)) | ||
| self.conv1.weight.data[2,0] = nn.Parameter(torch.tensor(-f2, dtype=torch.float32)) |
There was a problem hiding this comment.
Out of curiosity: couldn't this Linear-Nonlinear model have our Linear model as its linear component? Or is it attempting to model something different.
Eventually I think this (and Linear) should have docstrings that explain what they are and everything, but doesn't need to be part of this PR
There was a problem hiding this comment.
This is a toy model, not seriously aiming to do anything else but be a base case that is easy to understand and can be useful for development / debugging. If we decide to vamp it up, I propose we do so in a dedicated model_zoo branch
we're about to delete existing texture stats implementation, and this test was failing fraction_removed is tested elsewhere
…tionalVision/plenoptic into tools_organization
plenoptic/simulate/canonical_computations/steerable_pyramid_freq.py
Outdated
Show resolved
Hide resolved
plenoptic/simulate/canonical_computations/steerable_pyramid_freq.py
Outdated
Show resolved
Hide resolved
renames VentralModel to PooledVentralStream, RetinalGanglionCells to PooledRGC, and PrimaryVisualCortex to PooledV1, to clarify what they do Also updates the first line of each docstring
|
Major changes to structure of the repository and how tools and utility functions are handled. Major changes to steerable pyramid code to include not downsampled versions, fixes for reconstruction and tight frame constraints, steering function, preliminary tutorial. Also heavy documentation updates. |
Here we start organizing tools and tutorials. There is more to do.