ivy icon indicating copy to clipboard operation
ivy copied to clipboard

add hardsigmoid to Pytorch frontend

Open y-h-chan opened this issue 3 years ago • 2 comments

Closes #4263

y-h-chan avatar Sep 13 '22 11:09 y-h-chan

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 avatar Sep 13 '22 13:09 JamieLine

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()

JamieLine avatar Sep 13 '22 21:09 JamieLine

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

y-h-chan avatar Sep 19 '22 09:09 y-h-chan

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.

y-h-chan avatar Sep 20 '22 17:09 y-h-chan

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

JamieLine avatar Sep 20 '22 17:09 JamieLine

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.

y-h-chan avatar Sep 20 '22 17:09 y-h-chan

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.

JamieLine avatar Sep 20 '22 17:09 JamieLine

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

y-h-chan avatar Sep 20 '22 17:09 y-h-chan

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 avatar Sep 20 '22 17:09 JamieLine

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).

JamieLine avatar Sep 20 '22 18:09 JamieLine

its the comment, sorry let me quickly fix it.

y-h-chan avatar Sep 20 '22 18:09 y-h-chan

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 avatar Sep 22 '22 01:09 JamieLine