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

Adding Cos, Sin, Acos, Sigmoid to PyTorch Frontend #2471

Merged
merged 23 commits into from Aug 5, 2022

Conversation

pritam1322
Copy link
Contributor

@pritam1322 pritam1322 commented Jul 30, 2022

Add Pointwise ops to PyTorch Frontend #12

Copy link
Contributor

@iamjameskeane iamjameskeane left a comment

Choose a reason for hiding this comment

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

Hi can you please make the requested changes and re request a review :)

@@ -4,7 +4,7 @@
<option name="INTERPRETER_OPTIONS" value="" />
<option name="PARENT_ENVS" value="true" />
<option name="SDK_HOME" value="" />
<option name="WORKING_DIRECTORY" value="" />
<option name="WORKING_DIRECTORY" value="$PROJECT_DIR$" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove this change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@@ -4,7 +4,7 @@
<option name="INTERPRETER_OPTIONS" value="" />
<option name="PARENT_ENVS" value="true" />
<option name="SDK_HOME" value="" />
<option name="WORKING_DIRECTORY" value="" />
<option name="WORKING_DIRECTORY" value="$PROJECT_DIR$" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this changes also

@iamjameskeane iamjameskeane linked an issue Jul 31, 2022 that may be closed by this pull request
@pritam1322
Copy link
Contributor Author

ivy_tests/test_ivy/test_functional/test_core/test_elementwise.py
There are no change in this file, uploaded it by mistake.

@iamjameskeane
Copy link
Contributor

Hi @pritam1322,

It looks like you are implementing multiple functions yet you have only requested to do cos(). You would need to request to do the other functions from here before pushing their implementations. Alternatively you can just implement cos in this PR and then request to do others after it has been merged :)

Thanks,
James

@pritam1322
Copy link
Contributor Author

pritam1322 commented Aug 1, 2022

cos #2467
sin #2533
acos #2535
sigmoid #2480

@pritam1322 pritam1322 mentioned this pull request Aug 1, 2022
@pritam1322 pritam1322 closed this Aug 2, 2022
@pritam1322 pritam1322 reopened this Aug 2, 2022
Copy link
Contributor

@iamjameskeane iamjameskeane left a comment

Choose a reason for hiding this comment

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

In this PR you are still deleting the contents of file in .idea. In order to correct this you need to revert them back to how they are on Ivy/master locally and push the change.

Once you have done this request a review and I will review your implementations

@@ -1,2175 +0,0 @@
"""Collection of tests for elementwise functions."""
Copy link
Contributor

Choose a reason for hiding this comment

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

In this PR this is still a change to delete the contents of this file. Please undo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't undo it. Should I add it again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1,48 +0,0 @@
<component name="ProjectRunConfigurationManager">
Copy link
Contributor

Choose a reason for hiding this comment

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

In this PR this is still a change to delete the contents of this file. Please undo.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still an issue you need to revert this file to how it was on Ivy/master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1,48 +0,0 @@
<component name="ProjectRunConfigurationManager">
Copy link
Contributor

Choose a reason for hiding this comment

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

In this PR this is still a change to delete the contents of this file. Please undo.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still an issue you need to revert this file to how it was on Ivy/master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@iamjameskeane iamjameskeane changed the title Added cos and sin Pointwise ops to PyTorch Frontend Adding Cos, Sin, Acos, Sigmoid to PyTorch Frontend Aug 2, 2022
This was linked to issues Aug 2, 2022
Copy link
Contributor

@iamjameskeane iamjameskeane left a comment

Choose a reason for hiding this comment

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

Hi,

You have some lint errors with this commit. You can install a pre-commit hook as documented here. To prevent these errors in the future. The issues are highlighted in the GitHub action here. Please make the corrections for any files that are included in this commit.

Finally there are test failures for the functions you have implemented here. You need to have the tests passing before I can merge to master.

Thanks,
James

Copy link
Contributor

@iamjameskeane iamjameskeane left a comment

Choose a reason for hiding this comment

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

Please see requested changes :). I am also seeing some lint errors regarding the files you have been working on, if you haven't already please install a pre commit hook as documented here. This will prevent any lint errors.


def sigmoid(input, out=None):
return ivy.sigmoid(input, out=out)

Copy link
Contributor

Choose a reason for hiding this comment

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

add float 16 as unsupported data type here

):
input_dtype, x = dtype_and_x

if input_dtype == "float16":
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't skip any testing scenarios in the testing body. Please remove. See above for fix for this data type.

@pritam1322
Copy link
Contributor Author

pritam1322 commented Aug 4, 2022

Please see requested changes :). I am also seeing some lint errors regarding the files you have been working on, if you haven't already please install a pre commit hook as documented here. This will prevent any lint errors.

I have installed pre-commit and it gets check during push, but still getting lint error

@pritam1322
Copy link
Contributor Author

Should I create separate PR for each function? Things are getting mixed up.

@pritam1322
Copy link
Contributor Author

Lint run successfully

@iamjameskeane
Copy link
Contributor

Yep this looks good to me and all tests passing.

Thanks!

@iamjameskeane iamjameskeane merged commit 6115f87 into Transpile-AI:master Aug 5, 2022
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.

acos sin sigmoid cos
2 participants