Skip to content

Refactored Eth_Mnist #273

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.

Sign up for GitHub

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

Closed
coopersigrist wants to merge 2 commits into from
Closed

Refactored Eth_Mnist #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e19d72
refactored eth_mnist script and set default value for encoding in tor…
coopersigrist Jun 4, 2019
a350622
Merge pull request #3 from coopersigrist/data_fixes
coopersigrist Jun 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bindsnet/datasets/torchvision_wrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ class torchvision_dataset_wrapper(ds_type):

def __init__(
self,
image_encoder: Optional[Encoder],
label_encoder: Optional[Encoder],
image_encoder: Optional[Encoder] = None,
label_encoder: Optional[Encoder] = None,

@k-chaney k-chaney Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be None as the order of the arguments matter in this case. The point is to split the requirements. First we take care of Bindsnet requirements image_encoder and label_encoder then we take care of specific dataset requirements *args and **kwargs. I think forcing these (even if somebody puts None) keeps the consistency. @djsaunde thoughts?

@djsaunde djsaunde Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. @coopersigrist can you remove this?

@Hananel-Hazan Hananel-Hazan Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make sense, the requirements need to be split.
But, the default of the encoders need to be None, if the specific dataset require different encoder it will be specified later on in *args and **kwargs

@coopersigrist coopersigrist Jun 6, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to seperate them while keeping a default value? I agree that it's not necessary at all to make this change so if the preferred style is no default values I think we should definitely change it back.

@djsaunde djsaunde Jun 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hananel-Hazan The problem is not that the default Encoder should be None (it should be, and still is!), it's that if no arguments are specified for image_encoder and label_encoder, then the TorchvisionDatasetWrapper constructor won't know which argument to pass for the required root argument of the dataset class it wraps.

@djsaunde djsaunde Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k-chaney What about the following?

def __init__(
            self,
            *args,
            image_encoder: Optional[Encoder] = None,
            label_encoder: Optional[Encoder] = None,
            **kwargs
        )

@k-chaney k-chaney Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something important to remember is that this is a wrapper only for the datasets provided by torchvision. There won't be any other formats and one shouldn't arbitrarily use this wrapper on other datasets. Second, if a default is provided for image_encoder and label_encoder without moving them to the keyword section of the function signature you'll see the following. In this toy example I mirror what is seen in the proposed argument structure:

In [1]: def foo(a=None, *args, **kwargs):
   ...:     print(a)
   ...:     print(args)
   ...:     print(kwargs)
   ...:

Most of the time people would provide an argument for a and you would see (this is ideal case):

In [2]: foo('bindsnet_arg', 'torch_arg1', 'torch_arg2', torch='arg3')
bindsnet_arg
('torch_arg1', 'torch_arg2')
{'torch': 'arg3'}

But some people would skip providing an argument for a and that would result in the following:

In [3]: foo('torch_arg1', 'torch_arg2', torch='arg3')
torch_arg1
('torch_arg2',)
{'torch': 'arg3'}

Even further some people would provide a as a keyword argument later on:

In [4]: foo('torch_arg', 'torch_arg', torch='arg', a='bindsnet_arg')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-11-a49618c19c9b> in <module>()
----> 1 foo('torch_arg', 'torch_arg', torch='arg', a='bindsnet_arg')

TypeError: foo() got multiple values for keyword argument 'a'

*args,
**kwargs
):
Expand Down
32 changes: 27 additions & 5 deletions examples/mnist/eth_mnist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
import numpy as np
import matplotlib.pyplot as plt

from torchvision import transforms

from time import time as t

from bindsnet.datasets import MNIST
from bindsnet.encoding import PoissonEncoder
from bindsnet.encoding import poisson_loader
from bindsnet.models import DiehlAndCook2015
from bindsnet.network.monitors import Monitor
Expand Down Expand Up @@ -80,11 +83,30 @@
network.add_monitor(inh_voltage_monitor, name="inh_voltage")

# Load MNIST data.
images, labels = MNIST(
path=os.path.join("..", "..", "data", "MNIST"), download=True
).get_train()
images = images.view(-1, 784)
images *= intensity
# images, labels
dataset = MNIST(
None,
None,
root=os.path.join("..", "..", "data", "MNIST"),
download=True,
transform=transforms.Compose(
[
transforms.ToTensor(),
transforms.Lambda(lambda x: x * intensity),
transforms.Lambda(lambda x: x.view(784)),
]
),
)
# .get_train()

images = []
labels = []

for i in range(len(dataset)):
sample = dataset[i]
images.append(sample["image"])
labels.append(sample["label"])

@k-chaney k-chaney Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how I feel about this refactor. It would be difficult for somebody to take this and apply it to a larger dataset because of the extraction of this data ahead of time. I'll leave the final call on this to somebody else though.

@djsaunde djsaunde Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this doesn't make sense. @coopersigrist You should pass a PoissonEncoder to the MNIST constructor, and then just iterate over it during training.

@Hananel-Hazan Hananel-Hazan Jun 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

@coopersigrist coopersigrist Jun 6, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I'm gonna start redoing this now. I'll close this request and make the changes


# Lazily encode data as Poisson spike trains.
data_loader = poisson_loader(data=images, time=time, dt=dt)
Expand Down