ivy
ivy copied to clipboard
Transpile-AI
Did up Degrees #3957
Hi @khaiprograms, if you are doing the frontend function task, then you also need to implement a test for the function you have implemented. Check these docs (https://lets-unify.ai/ivy/deep_dive/17_ivy_frontends_tests.html) for more information
Ok, it seems your test is failing. You should check the test-frontend-numpy / run-nightly-tests (pull_request) workflow and find the results from your test. (You must open the "Test Frontend Functions" tab). There are a lot of failing tests, so it might take a while to find it, but it is there. You can check the bottom of the output log to see a summary of what failed, and degree is there. Also, to save some time in getting the PR merged, you should also make sure that your code is passing lint checks (under lint / check-formatting (pull_request)). These should be trivial changes to the code that can be done in a couple of minutes.
hey @Oxbeefbabe im looking through the errors and it seems like its an attribute error like so "AttributeError: module 'ivy.functional.backends.numpy' has no attribute 'degrees'" so i was wondering if its a thing where I have to implement something at the backend or is it something else
Hi, I've had a closer look, and it turns out that Ivy currently doesn't have a degrees function which you can call. This is something that has already been designated to be added to a future release. To that end you have two options:
You can implement the functionality of degrees within your frontend function (conversion from radians to degrees simply involves taking the value and multiplying it by 180/pi). You would also add a # TODO: comment explaining what you had done and how your code should be replaced with ivy.rad2deg once it is implemented.
Otherwise, you can mark label the issue for this PR as "Next Release" and then choose another frontend function to work on.
Hope this helps, let me know what you decide to do :)
Right, so to clarify, if I were to continue working on the degrees feature (which I'm leaning towards), I would have just have to implement the math behind it? Do I have to call anything else? Like ivy.rad2deg in my current frontend function?
I'm not sure if rad2deg is implemented in Ivy yet. If it is, then it hasn't been listed in the docs. If you find that it is implemented, then just use rad2deg in your function.
If rad2deg is not implemented currently, then just implement the math in the front-end function yourself, and then add a todo comment explaining what you have done and that it should be replaced with rad2deg when it is ready.
hey @Oxbeefbabe i redid it by adding the math to it to be safe, but it is still failing the test. Is this supposed to happen? From there, I don't seem to see the degrees function being tested too so i'm not entirely sure why that is the case. I wanna ask your opinion on this as well.
There seem to be a lot of failures in the numpy frontends, which makes finding your function difficult. It was tested, but your code is failing. This is because you are calculating it wrong. To convert radians into degrees, you need to use the following formula deg = rad * (180 / pi). Looking at your code, it appears you are doing deg = pi * (deg / 180). I believe if you fix that, you should be good to go.
Couple of other notes, please leave a TODO comment within the function explaining what you have done in terms of the math and how it should be replaced in the future. We need this because otherwise it might slip under the radar in the future, and we want to make sure everything is consistent.
Also, it appears that you have pushed some changes with your run configurations. Can you please revert everything within the .idea folder that you have changed? In the future, before you commit, you can select what files you want to commit, and deselect changes that shouldn't be pushed to git.
Okay @Oxbeefbabe , I implemented a different way as well (using the math way), but the test that is failing seems to be an assertion error and something that relates to the backend? so I'm not entirely sure what else can be done.
Also I sincerely apologise for the big push error and the closing of the request, honestly I had no idea what happened, it was just a fever dream of trying to get it to work.
Let me know what else can be implemented here, and also with regards to the TODO, I get an error and if I do write it down it will fail the lint check, so I'm kinda stumped there too.
Hi, I'm not sure what is causing the failure, but it is part of the test_frontend_function (the part where the value from your function and ground truth are compared). The data types appear to be different. I know that sometimes, NumPy can change data types to set default. That might be the case.
As for the to-do comment. If it is throwing lint errors for having the TODO comment (are you adding a space after #? i.e # TODO: blah blah blah) then you should be fine just adding a regular comment, not including the TODO keyword.
Keep in mind that you can run the tests for your function locally. If you are using PyCharm (which I assume you are because you have accidentally included some of the .idea files in your commit), you should be able to open the test_function, and to the left of the def keyword for the function, there should be a small play button. If you click on that and run normally, it will run the test on its own. This will run with no backends, however, so you will need to edit the new configuration that was created (should have the same name as the test) and in additional arguments, you need to add: --backend all. This should run your test function with all of the backends, and give you some immediate feedback about your test specifically. Should save you some time instead of waiting for the GitHub workflow results to come through and wading through that :).
Last couple of things: I mentioned you accidentally included some of the .idea files in your commit. Can you please remove these, as I can't merge until you do? There should be no changes in your PR listed under the folder .idea. Also, there seems to be a merge conflict between your branch and the master branch of Ivy. Can you please also resolve the conflict, as this will make it easier and faster for me to accept your work?
Hi @khaiprograms,
Are you still working on this? I've noticed you've made a few commits after my last comment but nothing else. If you would like to take a look at what you have done it is best to leave a comment so I know you are looking for a review.
hey @Oxbeefbabe, yep i still am. Sorry for the delay though, I'm just starting school which is why its taking a longer time. I should be done by thursday if that is alright
Also, I'm thinking if having a call with you would help with all these. That is, if you don't mind. Do let me know!
Hi @khaiprograms, thanks a lot for your contribution!!
I see that degrees has the same functionality as rad2deg, how about you add a simple one-liner binding to it?
This PR has been labelled as stale because it has been inactive for more than 7 days. If you would like to continue working on this PR, then please add another comment or this PR will be closed in 7 days.
This PR has been closed because it has been marked as stale for more than 7 days with no activity.