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.

Sign up for GitHub

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

Jump to bottom

refactored l1_loss and margin_ranking_loss in paddle frontend #22940

Closed
wants to merge 22 commits into from
Closed

refactored l1_loss and margin_ranking_loss in paddle frontend #22940

wants to merge 22 commits into from

Conversation

zeus2x7
Copy link
Contributor

@zeus2x7 zeus2x7 commented Sep 2, 2023 •
edited
Loading

refactored l1_loss,nll_loss and margin_ranking_loss in paddle frontend

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

Thanks for contributing to Ivy! 😊👏
Here are some of the important points from our Contributing Guidelines 📝:
1. Feel free to ignore the run_tests (1), run_tests (2), … jobs, and only look at the display_test_results job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that are passing on main, but fail on the PR Fork. Please try to make sure that there are no such tests. 💪
2. The lint / Check formatting / check-formatting tests check for the formatting of your code. 📜 If it fails, please check the exact error message in the logs and fix the same. ⚠️🔧
3. Finally, the test-docstrings / run-docstring-tests check for the changes made in docstrings of the functions. This may be skipped, as well. 📚
Happy coding! 🎉👨‍💻

@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Sep 2, 2023
@zeus2x7
Copy link
Contributor Author

zeus2x7 commented Sep 2, 2023

hi @lucasalavapena can you please review the pr?

@zeus2x7 zeus2x7 changed the title added normal function for paddle frontend refactored l1_loss in paddle frontend and added normal function for paddle frontend Sep 2, 2023
@zeus2x7 zeus2x7 changed the title refactored l1_loss in paddle frontend and added normal function for paddle frontend refactored l1_loss and margin_ranking_loss in paddle frontend and added normal function for paddle frontend Sep 2, 2023
@zeus2x7
Copy link
Contributor Author

zeus2x7 commented Sep 3, 2023

hi @lucasalavapena it would be much helpful if you could review the pr

@lucasalavapena
Copy link
Contributor

Hi @zeus2x7 . Thanks for your PR 😄 . Sorry for the wait, it was the weekend.

Could you please split your PR into 2? It is the commended approach per https://unify.ai/docs/ivy/overview/contributing/the_basics.html#small-commits-often . Ideally you could make one PR for the refactor and one into adding the normal function? You can assign me to both if you want as I will anyways now comment on both.

@zeus2x7 zeus2x7 changed the title refactored l1_loss and margin_ranking_loss in paddle frontend and added normal function for paddle frontend refactored l1_loss and margin_ranking_loss in paddle frontend Sep 4, 2023
@ivy-leaves
Copy link

If you are working on an open task, please edit the PR description to link to the issue you've created.

For more information, please check ToDo List Issues Guide.

Thank you 🤗

@lucasalavapena lucasalavapena mentioned this pull request Sep 4, 2023
Merged
@zeus2x7
Copy link
Contributor Author

zeus2x7 commented Sep 4, 2023

hi @lucasalavapena can you please review the pr again?
have made all the changes suggested by you

@zeus2x7
Copy link
Contributor Author

zeus2x7 commented Sep 5, 2023

@lucasalavapena could you please review this pr also

@lucasalavapena
Copy link
Contributor

Please have some patience.

@lucasalavapena
Copy link
Contributor

Btw a general comment, something like nll_loss should ideally be calling the ivy functional api, but that function has not been implemented yet and of course you are simply completing/refactoring it!

@zeus2x7
Copy link
Contributor Author

zeus2x7 commented Sep 5, 2023

hi @lucasalavapena can you please give it a look now?

lucasalavapena
lucasalavapena reviewed Sep 6, 2023
View reviewed changes
Copy link
Contributor

@lucasalavapena lucasalavapena left a comment

Choose a reason for hiding this comment

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

Please ensure that all tests related to this PR pass, test_paddle_nll_loss is failing (see the combined test results section of display test results). I first realised this when I ran it in codespaces because I was confused how it would be passing given that log or log_softmax is applied here.

ivy/functional/frontends/paddle/nn/functional/loss.py Outdated
if weight is None:
weight = ivy.ones(ivy.shape(input[0]))
input = ivy.log(input)
input = ivy.log_softmax(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

log_softmax does seem more correct than what was previously there, however, looking at the paddle docs: [0] and [1] it is expected that the input is already log prob (see the example in the first link)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also in general please name input something else if you transform it, that could be a bit confusing if not read in a linear manner

@zeus2x7 zeus2x7 closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucasalavapena lucasalavapena lucasalavapena left review comments

Assignees

@lucasalavapena lucasalavapena

Labels
Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zeus2x7 @lucasalavapena @ivy-leaves