Skip to content
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

remove the hard torch library dependecies #10

Merged
merged 3 commits into from
Oct 10, 2020
Merged

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Oct 10, 2020

#9

Get rid of importing torch.Size by using type(v.shape).name instead/

@noklam
Copy link
Contributor Author

noklam commented Oct 10, 2020

@parrt
I try run through the test, it seems working fine for the torch example.
The downside of using type(v.shape).name is it doesn't handle the inheritance problem. But I am not aware of any other method to get rid of checking the object without actually importing

@parrt parrt added compatibility enhancement New feature or request labels Oct 10, 2020
@parrt parrt added this to the 0.1b4 milestone Oct 10, 2020
@parrt
Copy link
Owner

parrt commented Oct 10, 2020

Hi @noklam thanks very much for your help! Maybe instead of if isinstance(v.shape, torch.Size), we ask for v.shape.__class__.__name__=='Size' or whatever?

@noklam
Copy link
Contributor Author

noklam commented Oct 10, 2020

I don't know if there is any difference between type(v.shape).__name__ versus v.shape.__class__.__name__

https://stackoverflow.com/questions/510972/getting-the-class-name-of-an-instance
Some suggesting it is a bit different in Python 2, both are fine!

@parrt

@parrt
Copy link
Owner

parrt commented Oct 10, 2020

Yeah, I guess you're right. I wonder if we need to check if module is torch and name is Shape.

Re inheritance: likely most people are not subclassing torch tensors. if so, though, wouldn't Shape get inherited into subclass and our test would work?

@noklam
Copy link
Contributor Author

noklam commented Oct 10, 2020

@parrt Yes, I think it should still work. I think you mean name is Size ?

p.s. I know fastai is subclassing torch tensor. :) https://github.com/fastai/fastai/blob/e34fee816f5dcbf05a9bc9b44e617902a52549fc/fastai/torch_core.py#L300

Btw the test1.py is failing

with dbg():
    z = torch.sigmoid(self.Whz @ h + self.Uxz @ x + self.bz)
    b = W @ b + np.abs(x)

I am getting

NameError: name 'self' is not defined

@noklam
Copy link
Contributor Author

noklam commented Oct 10, 2020

Added the checking for module and name

@parrt
Copy link
Owner

parrt commented Oct 10, 2020

yeah test?.py are just junk drawers. deleted test1.py

yep, Shape=>Size

@parrt
Copy link
Owner

parrt commented Oct 10, 2020

Looks great! Merging.

@parrt
Copy link
Owner

parrt commented Oct 10, 2020 via email

@noklam
Copy link
Contributor Author

noklam commented Oct 11, 2020

Thanks it works! Can you add it to #3 too?

@parrt
Copy link
Owner

parrt commented Oct 11, 2020 via email

@noklam
Copy link
Contributor Author

noklam commented Oct 11, 2020

@parrt Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants