Conversation
…o, and contributing document
…wise. Noted that this repo is implemented in PyTorch w GPU support
|
autopep8? |
9q9q
left a comment
9q9q
left a comment
There was a problem hiding this comment.
thanks alex!! just a few small comments
fixed typos in PyTorchOptimizer line
…o formatting pull changes
|
@alvinz-matician I'm unfamiliar with autopep8. Would this be something that would be locally run before pushing code, or automated through github actions? |
|
It's a Python auto-formatter for PEP8 standards, I just wanted to put it on your radar. I was thinking that it could be nice to add as a PR check in GitHub actions. |
|
from a quick search, it looks like flake8 wraps a few things including pyflakes (statically checks for logical errors) and pycodestyle (formerly pep8). |
alvinzz
force-pushed
the
formatting
branch
2 times, most recently
from
936c04b to
a49b3ee
Compare
|
All new changes look good to me. |
|
|
||
| def whiten_dataset(dataset,plot=False,title='',n_comp=-1): | ||
| def whiten_dataset(dataset, plot=False, title='', n_comp=-1): | ||
| from torchsmodel import sparsecoding |
There was a problem hiding this comment.
This isn't in the repo. I think it's code from your previous implementation @belsten. It's called later in the code too. How is it passing tests? Are you perhaps not calling this function?
| def whiten_dataset(dataset,plot=False,title='',n_comp=-1): | ||
| def whiten_dataset(dataset, plot=False, title='', n_comp=-1): | ||
| from torchsmodel import sparsecoding | ||
| from utils import plotmontage |
There was a problem hiding this comment.
Move to the top with the other imports.
|
|
||
|
|
||
| def nptotorch(x,device=torch.device('cpu'),dtype=np.float32): | ||
| def nptotorch(x, device=torch.device('cpu'), dtype=np.float32): |
There was a problem hiding this comment.
nptotorch -> np_to_torch
There was a problem hiding this comment.
Do we need this function?
You can also say
torch.tensor(v, device=device, dtype=dtype)
See torch.torch
https://pytorch.org/docs/stable/generated/torch.tensor.html
| # NATURAL IMAGES | ||
| ## ================================================================================ | ||
| # ================================================================================ | ||
| class naturalscenes(Dataset): |
There was a problem hiding this comment.
Class names should be capitalized.
class NaturalScenes(Dataset):
...
|
|
||
| return self.patches[idx, :] | ||
|
|
||
| def extractpatches(self): |
There was a problem hiding this comment.
This function will be generally useful beyond just NaturalScenes. We can go ahead and include this in the merge, but I think we should instead put this into a data utils module, then import it for use here rather than defining it within the NaturalScenes class.
There was a problem hiding this comment.
Also, the name should be snake case:
def extract_patches
| plotmontage(model, color=True, size=10, title=title) | ||
|
|
||
| # whiten | ||
| trotpatch = (nptotorch(v, device=dataset.device).t()@dataset.patches.t()).t() |
There was a problem hiding this comment.
variable names should follow snake-case convention.
t_rot_patch
|
I'm rewriting the natural scenes and patches stuff in #30 so IMO it's fine for now. At least until we have a writeup I don't think we'll have too much external traffic. |
|
Yep. Realized that after I looked at #30. Thanks! |
|
Can these requests be resolved so we can merge this branch? |
5 participants
No description provided.