-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
implement numpy ufuncs #249
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #249 +/- ##
==========================================
- Coverage 95.77% 94.86% -0.91%
==========================================
Files 15 17 +2
Lines 1916 2123 +207
==========================================
+ Hits 1835 2014 +179
- Misses 81 109 +28 ☔ View full report in Codecov by Sentry. |
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.
Can you add some description of what the changes are here and what the motivation for them is?
uncertainties/core.py
Outdated
@@ -332,7 +334,7 @@ def __setstate__(self, state): | |||
(self.linear_combo,) = state | |||
|
|||
|
|||
class AffineScalarFunc(object): | |||
class AffineScalarFunc(UFloatNumpy): |
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.
This makes AffineScalarFunc
and thus all of uncertainties
dependent on numpy which I don't think has been discussed recently? The only previous discussion I can find is #47 where the conclusion seemed to be that improving numpy support should go into uarray
rather than AffineScalarFunc
/UFloat
which makes sense to me.
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 agree with @wshanks. AffineScalarFunc
should not require numpy.
It is a scalar, it does not need broadcasted ufuncs.
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.
this is how to get np.sin(x) to work, removing the need for unumpy.sin
I meant to make UFloatNumpy an empty class when numpy is not installed. Got a bit trigger happy when finally getting the tests to pass! (The ufuncs get used when a AffineScalarFunc
/UFloat
gets multiplied, added etc by a np.array containing anything)
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.
we also wouldn't need a umath.sin, although that may be worth keeping so users don't need to have numpy installed
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.
@andrewgsavage I have not looked in detail at this. I think I may not really understand your responses.
Are you saying that the thing called UFloatNumpy
will not depend on numpy? That is going to be super-confusing. Should this class use numpy
if available and math
otherwise? Maybe call that UMath
?
Again agreeing with @wshanks please you give an overview of the design goals and concepts, or a link to an earlier discussion.
The end goal here is to make uncertainties useable with numpy without using unumpy, which is one of the things I've been meaning by improved support for numpy. This PR lets you do:
Where as in master you'll get this error:
And the same for all the other ufuncs listed here: https://github.com/lmfit/uncertainties/pull/249/files#diff-e46d8afbe21c61c3f19c1efe9849800eb86ba3a5570bd40116e866d8b3c92740R168 |
@andrewgsavage I wonder if this code should wait until after |
@andrewgsavage I like that goal of letting |
Yes
I don't think there's any way to overide math functions so unfortunately not.
"make uncertainties useable with numpy" does mean making numpy scalar functions work. It'd be annoying to have to use umath.sin or np.sin depending on whether its operating on a scalar or array. Yes Implementing Yea there's quite a few steps to get everything. Probably worth a discussion topic + checklist for numpy support alone |
That's fine, enjoy them! I think it can, although doing it like that isn't something I've seen elsewhere. It would be good to use the same approach for ops and numpy though. |
Yea maybe, lets see how fast that moves. I think it should just be class names changing that'd affect this. |
@andrewgsavage to implement Here's how I've done it on my rewrite:
The big advantage of having |
pre-commit run --all-files
with no errors