-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
smooth_l1_loss #22378
smooth_l1_loss #22378
Conversation
Thanks for contributing to Ivy! 😊👏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I suppose you need to implement the backend of your function for every framework available to allow your function to pass tests, not just paddle and torch.
by the way, you should have a look at the new guide line of how we should write commit messages: https://www.conventionalcommits.org/en/v1.0.0/. Thanks. |
@Daniel4078 The problem here is smooth_l1_loss is and mixed function due to that only some backends have the implementation of smooth_l1_loss,oneis torch and paddle ,so that is the reason have implemented in this way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that you should still try to implement this function compositionally in other backend by recreating its intended behavior using combinations of other functions in the target backend framework.
i will work on it |
@Daniel4078 I have implemented the backend support manually in respective frameworks of jax,numpy and tensorflow...can u pls review that,it might have some mistakes over them,pls review it once and let me knew any changes are needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update your branch with the main one as I found that your tests are failing due to an old problem: E ModuleNotFoundError: No module named 'dill'. Which has been patched out in recent commits on the ivy main branch.
@Daniel4078 i have updated it and pushed the code..Now tests are passing as u mentiioned screenshot of it is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it will be good for you slightly expand your docstring examples for your function like guided here(https://unify.ai/docs/ivy/overview/deep_dive/docstring_examples.html#docstring-examples). For example, some example requarding the use of reduction and out argument will be appreciated.
@Daniel4078 i have changed the doc strings any new changes are needed?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a failing test of your function, please have a look:
E ivy.utils.exceptions.IvyBackendException: paddle: execute_with_gradients: paddle: nested_map: paddle: nested_map: (Fatal) There is no grad op for inputs:[0] or it'sstop_gradient=True.
E [Hint: target_node should not be null.] (at /paddle/paddle/fluid/eager/general_grad.h:72)
E
E Falsifying example: test_smooth_l1_loss(
E backend_fw='paddle',
E on_device='cpu',
E dtype_and_input=(['float64'], [array([-1., -1., -1.])]),
E dtype_and_target=(['float64'], [array([-1., -1., -1.])]),
E beta=0.5,
E reduction='none',
E test_flags=FunctionTestFlags(
E ground_truth_backend='tensorflow',
E num_positional_args=2,
E with_out=False,
E instance_method=False,
E test_gradients=True,
E test_compile=False,
E as_variable=[False],
E native_arrays=[False],
E container=[False],
E ),
E fn_name='smooth_l1_loss',
E )
@Daniel4078 it has been failing for all of the loss function that is paddle bfloat16 issue or some backend issue. |
@Daniel4078 here is the screenshot of it. |
Maybe you can leave a comment in the relevent code saying this problem, then I can merge and let other ivy members to deal with this global problem. |
@Daniel4078 i have added in the doc string mentioning the error..can u pls review it ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me!
Co-authored-by: Daniel4078 <[email protected]>
close #21165