Refactor power derivative functions #294
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #294 +/- ##
==========================================
+ Coverage 86.47% 86.54% +0.07%
==========================================
Files 18 18
Lines 1893 1903 +10
==========================================
+ Hits 1637 1647 +10
Misses 256 256
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #294 will not alter performanceComparing Summary
|
| @@ -119,29 +141,6 @@ def get_ops_with_reflection(): | |||
| eval("lambda y, x: %s" % expr) for expr in reversed(derivatives) | |||
| ] | |||
|
|
|||
| # The derivatives of pow() are more complicated: | |||
|
|
|||
| # The case x**y is constant one the line x = 0 and in y = 0; | |||
There was a problem hiding this comment.
it's a shame to lose this comment. is there somewhere it could be moved to? I would say a docstring but that's not useful for __pow__.
maybe it could be added to umath.pow's docstring?
There was a problem hiding this comment.
Let me take a look. I actually spent a VERY long time puzzling over various special cases and I had some rewrites of this same comment.
Part of the problem is that the old function handled different special cases using different approaches. For example, the old function allowed complex results to be returned but, instead, relied on some other part of the codebase to prevent complex results.
Under this PR the comment is not actually correct anymore but I can come up with replacement text. But I'm curious, do you want a comment for the developer's sake (like the old comment used to be) or for the user's sake? For the latter we can add some description under the umath.pow docstring. We can also add documentation to the docs webpage. If we want a comment for the developers sake then I can update the old comments with some notes about the various special cases and how they're handled.
Also happy to do all of the above, though it ends up being a lot of text because there's a few different cases.
There was a problem hiding this comment.
I think it's most useful for users so a docstring or section of the documentation, either works.
|
@andrewgsavage @newville I've included words and codeblocks in the user guide documentation near the nan/inf discussion. Let me know what you think. There are more cases that could be demonstrated. I also intentionally left out examples with Are more cases needed to show the different situations? Do we want any of this to appear on the docstring for I'm relatively happy where it is, but open to suggestions. |
|
seems good |
|
Cleaned it up a little more. Think it's ready for approval and merging if others agree. |
|
Can someone please approve and merge #302 and then merge this PR? @newville @andrewgsavage? After that we are ready to release version |
There was a problem hiding this comment.
@jagerber48 @andrewgsavage This looks good to me... Any objections to merging?
|
go for it |
pre-commit run --all-fileswith no errorsuncertaintiesallows calculatingx**ywhere one or both ofxandyareUFloatobjects. For this we need to calculate thexandyderivatives ofx**y. This PR refactors the functions that calculate those derivatives to more explicitly and clearly handle non-typical cases. Moving those functions to the module level also allows these functions to be directly tested.