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

add hardsigmoid to Pytorch frontend #4409

Merged
merged 15 commits into from Sep 22, 2022
Merged

Conversation

y-h-chan
Copy link
Contributor

Closes #4263

@ivy-leaves ivy-leaves added the PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist label Sep 13, 2022
@JamieLine
Copy link
Contributor

Hi,

Thanks for the PR! I like whats there so far, that looks like a nice and elegant implementation. There are a few things that need to be added though.

1.) It needs to have a test written in ivy_tests/test_ivy/test_frontends/test_torch/test_non_linear_activation_functions.py, there are other tests in that directory to use as examples, but to help this code get through review quickly, here is a useful tip.

  • Use helpers.get_dtypes(X) where X is one of "valid", "numeric", "float" or similar to specify the data types this function takes as an input, and try to use "valid" first, only using one of the others if it is needed to make the test work.

2.) This function needs to have a matching function signature to the equivalent PyTorch function, so as the original is (I think) torch.nn.functional.hardsigmoid the Ivy version must be ivy.functional.frontends.torch.nn.functional.hardsigmoid.

I'd like to re-iterate I do like what's there now and think it'll be great when the rest has been added, and if there's anything you need help with feel free to message me.

@JamieLine
Copy link
Contributor

The test looks mostly correct but it fails on my machine with TypeError: hardsigmoid() got an unexpected keyword argument 'threshold', this is because the end of the call to helpers.test_frontend_function is wrong, every keyword argument after fn_tree in this call will be passed to the function that's being tested. Also you've specified inplace as always being false, this should be generated in the @given decorator, using st.booleans()

@y-h-chan
Copy link
Contributor Author

Should i merge after the workflow or i should keep it up to date?

@y-h-chan
Copy link
Contributor Author

May i get a pointer on proper implementation of the test method? The test in pytest failed but I believe the implementation is correct as the results are correct on manual testing on both the ivy frontend and the torch backend.

@JamieLine
Copy link
Contributor

Hi, what is the error you get in pytest? It looks alright at a first glance, I'll pull this version and see if it works for me

@y-h-chan
Copy link
Contributor Author

i guess its the inplace behaviour of the function changing the input. So im not sure if the helper functions are intended to test with inplace.

@JamieLine
Copy link
Contributor

Having got it running that makes sense, I had a test failure where the values disagreed and one of them was correctly hardsigmoid(x) and one was incorrectly hardsigmoid(hardsigmoid(x)), I'll quickly see how this is usually avoided.

@y-h-chan
Copy link
Contributor Author

Other tests simply ignore testing the inplace but this seems weird.

@JamieLine
Copy link
Contributor

It looks like the other functions just pass False in, so I'd say just do that and add a comment that says it was validated to work outside of the testing framework, but causes errors in the testing framework.

Also very small thing, but when you update to inplace=False, the spaces should be removed around the = character, it's a minor thing but its the standard style in the codebase to remove the spaces when assigning keyword arguments.

@JamieLine
Copy link
Contributor

Having had a quick once-over, try running flake8 and black on the files you're working on, as is typically done by pre-commit. I think there's at least one line that goes over the typical maximum length (which I believe is 88 characters).

@y-h-chan
Copy link
Contributor Author

its the comment, sorry let me quickly fix it.

@JamieLine
Copy link
Contributor

Thanks for fixing it, I fixed a merge conflict against master and re-ran the formatting tools, so if you see my commit in your branch there's nothing to be alarmed about, and it doesn't change that this is your commit and you still receive full value out of it, I just felt it was unfair for me to introduce a formatting issue and ask you to fix it.

@JamieLine JamieLine merged commit 97828c1 into Transpile-AI:master Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hardsigmoid
3 participants