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

fix(FixSigMeta): remove self from signature #24

Merged
merged 1 commit into from Apr 8, 2020

Conversation

borisdayma
Copy link
Contributor

Remove self from signature using FixSigMeta.

This issue came from a user reporting a log_args error on LabelSmoothingEntropy (meaning that parameters on this function were not saved).

I noticed that the signature included self, then looked at Module which uses PrePostInitMeta, itself inheriting from FixSigMeta. Was tricky to solve but I think this should do it!

In theory there should never be a message from log_args unless there's a signature issue.

The only signature issues not shown are when too many parameters are passed because **kwargs is typically removed while actually everything is passed. In that case the parameter is named function.parameter (not in signature).

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@sgugger
Copy link
Contributor

sgugger commented Apr 8, 2020

Did you check all the tests in fastcore pass with this change? I'm a bit nervous to change a function that is at the base of everything.

@borisdayma
Copy link
Contributor Author

Good point, I checked only the specific notebook but I should run all the tests of fastcore and fastai2.

My understanding of this specific line is that it's barely for displaying the help of the function. If we remove this line it shows something completely different since we overwrite the new method.

I'll check all the tests and will get back to you.

@sgugger
Copy link
Contributor

sgugger commented Apr 8, 2020

It might impact other functions that rely on the signature, but if that notebook passes, all should be good. Just run make test in the repo to quickly test everything in parallel.

@borisdayma
Copy link
Contributor Author

Yes they all passed on fastcore!

@sgugger
Copy link
Contributor

sgugger commented Apr 8, 2020

Ok, let's merge this, thanks!

@sgugger sgugger merged commit 2856372 into fastai:master Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants