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.

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

andrewgsavage
Copy link
Contributor

  • Closes # (insert issue number)
  • Executed pre-commit run --all-files with no errors
  • The change is fully covered by automated unit tests
  • Documented in docs/ as appropriate
  • Added an entry to the CHANGES file

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 26 lines in your changes missing coverage. Please review.

Project coverage is 94.86%. Comparing base (64e7320) to head (f45193a).

Files Patch % Lines
uncertainties/ufloatnumpy.py 77.57% 24 Missing ⚠️
uncertainties/core.py 77.77% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@wshanks wshanks left a 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?

@@ -332,7 +334,7 @@ def __setstate__(self, state):
(self.linear_combo,) = state


class AffineScalarFunc(object):
class AffineScalarFunc(UFloatNumpy):
Copy link
Collaborator

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.

Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

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

Copy link
Member

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.

@andrewgsavage
Copy link
Contributor Author

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:

np.sin(ufloat(1,1))
0.8414709848078965+/-0.5403023058681398

Where as in master you'll get this error:

np.sin(ufloat(1,1))
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
AttributeError: 'Variable' object has no attribute 'sin'

The above exception was the direct cause of the following exception:

TypeError                                 Traceback (most recent call last)
Cell In[4], line 1
----> 1 np.sin(ufloat(1,1))

TypeError: loop of ufunc does not support argument 0 of type Variable which has no callable sin method

And the same for all the other ufuncs listed here: https://github.com/lmfit/uncertainties/pull/249/files#diff-e46d8afbe21c61c3f19c1efe9849800eb86ba3a5570bd40116e866d8b3c92740R168

@jagerber48
Copy link
Contributor

@andrewgsavage I wonder if this code should wait until after AffineScalarFunc is refactored. I wonder if there might be coupling, and it may be easier to implement the numpy protocol after that refactoring. I want to look more closely at this PR when I get time.

@wshanks
Copy link
Collaborator

wshanks commented Jul 13, 2024

@andrewgsavage I like that goal of letting numpy.sin etc just work on AffineScalarFunc. I would need to do some background reading on numpy ufuncs to review this though. I may not have much time over the next month due to upcoming vacations, but I don't have any general objections if others come to an agreement on how this should be formatted. I do wonder though if the ufunc support could be entirely encompassed in add_* methods the way other methods are added on to AffineScalarFunc (like ops.add_arithmetic_ops) without adding a new level in the class hierarchy. I am not sure if that is better though. If keeping the current form of an optional UFloatNumpy parent class, maybe the _add_numpy_*_ufuncs methods could be called on UFloatNumpy when it is defined instead of being called on AffineScalarFunc.

@newville
Copy link
Member

newville commented Jul 13, 2024

@andrewgsavage As you say, np.sin(ufloat(1, 1)) is not (and was not ever, I think) supported. Is that the main goal here?

Yes

Would it also be a goal to support math.sin(ufloat(1, 1))?

I don't think there's any way to overide math functions so unfortunately not.

I would guess that most users would expect np.sin() to work on numpy ndarrays. It's convenient that it also works on scalars, but that does not seem that important to me, given math.sin().

That is, "make uncertainties useable with numpy" could easily be interpreted as mostly about ndarrays. Ideally, calculations with ndarrays would not use Python loops but rely on numpy broadcasting. This PR includes a new function apply_func_elementwise that appears to be used to loop over array elements. Is that correct? Or are ndarrays of ufloats handled separately?

I might be missing some context that is clear to you. It feels awkward to ask for a discussion of basic goals in a PR from a core developer, but I think that would be helpful here.

"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 apply_func_elementwise is used to loop over any ndarray. There may be some better way to do this; I've been trying to get this to work first.

Implementing __array_ufunc__ overides the existing broadcasting that numpy does when there's a ndarray is the operation, so this needs to be able to handle arrays to allow np.add(ufloat, arr_of_ufloat). It would also be able to return a UArray class instead of ndarray_of_ufloats, once that class is implemented.

Yea there's quite a few steps to get everything. Probably worth a discussion topic + checklist for numpy support alone

@andrewgsavage
Copy link
Contributor Author

@andrewgsavage I like that goal of letting numpy.sin etc just work on AffineScalarFunc. I would need to do some background reading on numpy ufuncs to review this though. I may not have much time over the next month due to upcoming vacations, but I don't have any general objections if others come to an agreement on how this should be formatted. I do wonder though if the ufunc support could be entirely encompassed in add_* methods the way other methods are added on to AffineScalarFunc (like ops.add_arithmetic_ops) without adding a new level in the class hierarchy. I am not sure if that is better though. If keeping the current form of an optional UFloatNumpy parent class, maybe the _add_numpy_*_ufuncs methods could be called on UFloatNumpy when it is defined instead of being called on AffineScalarFunc.

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.

@andrewgsavage
Copy link
Contributor Author

@andrewgsavage I wonder if this code should wait until after AffineScalarFunc is refactored. I wonder if there might be coupling, and it may be easier to implement the numpy protocol after that refactoring. I want to look more closely at this PR when I get time.

Yea maybe, lets see how fast that moves. I think it should just be class names changing that'd affect this.

@jagerber48
Copy link
Contributor

@andrewgsavage to implement ufuncs on UFloat so that, e.g., np.sin(ufloat(3, 0.1)) works, why not simply define a sin() method on the UFloat class? Or, if we don't want to muddle up the source code, we could have UFloat inherit from a numeric class on which we define those methods.

Here's how I've done it on my rewrite:

The big advantage of having NumericBase over not having it is that it allows all of the monkey patched methods to show up with proper type annoations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants