8000 Eliminate the `Variable` class by jagerber48 · Pull Request #262 · lmfit/uncertainties · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Eliminate the Variable class #262

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 117 commits into
base: main
Choose a base branch
from
Open

Eliminate the Variable class #262

wants to merge 117 commits into from
+1,431 −2,054

Conversation

jagerber48
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

This PR:

  • Eliminates the Variable class and eliminates the AffineScalarFunc.derivatives property.
  • The AffineScalarFunc class is renamed to UFloat with AffineScalarFunc left as a legacy name.
  • The LinearCombo class is refactored into the UCombo class but the architectural role is similar: track the uncertainties of UFloat objects as a lazily expanded combination of objects with uncertainty
  • The UAtom object is introduced as the fundamental element which captures uncertainty. Each UAtom models a random variable with zero mean and unity standard deviation/variance. Each UAtom has a uuid and all UAtom are mutually statistically independent. The expanded version of the UCombo maps dict[UAtom, float].

For more detail/discussion see #251 (comment)

Note that as of the time opening this PR the PR is still a work in progress. Not all elements in the bullet list above are implemented and a good handful of tests are still failing.

@jagerber48 jagerber48 changed the title Feature/linear combo refactor Eliminate the Variable class Aug 12, 2024
jagerber48
jagerber48 commented Nov 16, 2024
View reviewed changes
tests/test_uncertainties.py Outdated
@@ -104,9 +105,6 @@ def test_ufloat_fromstr():
"3.4(nan)e10": (3.4e10, float("nan")),
# NaN value:
"nan+/-3.14e2": (float("nan"), 314),
# "Double-floats"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These floats are too large to work with. Prior to this PR the large float value could be saved into the AffineScalarFunc object and displayed, but any arithmetic with such large values would result in an OverflowError. In the new framework even displaying a UFloat the first time requires a calculation, even if it's as simple as sqrt(x**2). So in the new framework the OverflowError appears immediately.

@jagerber48 jagerber48 mentioned this pull request Dec 7, 2024
Open
@jagerber48
Copy link
Contributor Author

All the tests are passing except some having to do with the uarray inverse and pseudo inverse functions. The code for this is pretty complicated. It might make sense to just rewrite the code to convert array-compatible functions to ufloat array compatible functions.

@jagerber48 jagerber48 mentioned this pull request Dec 12, 2024
Open
5 tasks
@codecov Codecov
Copy link
codecov bot commented Dec 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.64593% with 60 lines in your changes missing coverage. Please review.

Project coverage is 85.43%. Comparing base (b175125) to head (6583987).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
uncertainties/unumpy/core.py 77.85% 31 Missing ⚠️
uncertainties/ucombo.py 80.61% 19 Missing ⚠️
tests/test_unumpy.py 0.00% 8 Missing ⚠️
tests/helpers.py 83.33% 1 Missing ⚠️
uncertainties/core.py 97.72% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
- Coverage   86.69%   85.43%   -1.27%     
==========================================
  Files          18       19       +1     
  Lines        1729     1847     +118     
==========================================
+ Hits         1499     1578      +79     
- Misses        230      269      +39
Flag Coverage Δ
macos-latest-3.10 90.04% <82.77%> (-3.96%) ⬇️
macos-latest-3.11 90.04% <82.77%> (-3.96%) ⬇️
macos-latest-3.12 90.04% <82.77%> (-3.96%) ⬇️
macos-latest-3.8 90.03% <82.77%> (-3.97%) ⬇️
macos-latest-3.9 90.03% <82.77%> (-3.97%) ⬇️
no-numpy 80.07% <62.91%> (-2.17%) ⬇️
ubuntu-latest-3.10 90.04% <82.77%> (-3.96%) ⬇️
ubuntu-latest-3.11 90.04% <82.77%> (-3.96%) ⬇️
ubuntu-latest-3.12 90.04% <82.77%> (-3.96%) ⬇️
ubuntu-latest-3.8 90.03% <82.77%> (-3.97%) ⬇️
ubuntu-latest-3.9 90.03% <82.77%> (-3.97%) ⬇️
windows-latest-3.10 90.04% <82.77%> (-3.96%) ⬇️
windows-latest-3.11 90.04% <82.77%> (-3.96%) ⬇️
windows-latest-3.12 90.04% <82.77%> (-3.96%) ⬇️
windows-latest-3.8 90.03% <82.77%> (-3.97%) ⬇️
windows-latest-3.9 90.03% <82.77%> (-3.97%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jagerber48
Copy link
Contributor Author
jagerber48 commented Dec 13, 2024 •
edited
Loading

Wow all tests are passing now. That's a big milestone for this PR. If folks are interested I'd be curious to hear general feedback on the source code changes in this PR. I tried to do as little wholesale rewriting as possible but did chose to do some of that in unumpy/core.py.

Here's a summary of the changes so far. See #251 for a more thorough discussion of these changes and their motivations.

  • Remove the Variable class.
  • Remove the LinearCombination class.
  • Add the UAtom class. A UAtom models a standard normally distributed random variable (zero mean, unity variance).
  • Add the UCombo class. This class essentially maps UAtoms to float. This class models a linear combination of UAtoms. Exposes a method to extract its own standard deviation and its covariance with other UCombos. This class supports multiplication by floats/ints and additions with other UCombo's. This simple arithmetic support makes uncertainty propagation calculations nice.
  • Swap the names UFloat and AffineScalarFunc. Now UFloat defines the actual class and AffineScalarFunc is an alias (perhaps it could be deprecated later).
  • The UFloat class now stores uncertainty as a UCombo rather than a LinearCombination. Previously uncertainty was roughly recorded as the derivatives of an AffineScalarFunc with respect to Variables. This information was stored and accessed in the derivatives attribute. Now there is no longer a derivatives attribute. There is only error_components which now returns a mapping from UAtom to float indicating the weighting of each UAtom within a UFloat. This is the biggest user-facing conceptual change in this PR.
  • It was relatively straightforward to give UFloat a simple hash. This will resolve Pandas sort_values with multiple columns does not work for AffineScalarFunc #186 and Should `Variable.__eq__` be based on `id()` or a separate identifier? #218 and Implement hash for AffineScalarFunc #219. I'll point out that these conversations were major concrete motivators for this refactor (together with a less concrete desire to simplify the codebase)
  • Previously AffineScalarFunc and Variable could be tagged with a user-readable tag. Now UFloats cannot be tagged, but UAtoms can.
  • Because of the previous architecture there were some confusing behaviors with respect to copying/pickling. In the new framework I think copying and pickling are much more straightforward. The behaviors are different than before, but I argue they are better. You can look at how the tests changed for more details onthis. See also Strange behavior on copying #260.
  • The code in unumpy/core.py responsible for extending np.linalg.inv and np.linalg.pinv to operate sensibly on umatrix or arrays of UFloat were rewritten. The old code (1) relied centrally on AffineScalarFunc.derivatives and (2) was quite confusing to understand. In this case I found it easier to rewrite the code than to understand and modify the old code. The old code could only uncertainty-ify functions that took one array as an argument. The new code detect UFloats or arrays containing UFloat's in any argument. By default it uses numerical differentiation, but the function can be passed functions to calculate analytical derivatives.

Some todo items

  • Check that performance isn't impacted
  • Multiple passes of clean up on the code
  • Some passes on the source code for the tests. Many tests were changed, some dramatically. Some tests were added. I think the tests as they are now are probably ok. I think some checks that this PR can go through would be good, but a more thorough addressing of tests should be done in support of resolving Clean up tests #273 in later PRs.
  • Correct and clean up comments within the source code. The legacy uncertainties code has a lot of comments and documentation. This PR is a part of a thrust to help simplify the source code so that it is a bit more self-documenting. With this, in the places touched by this PR, I want to slim down any comments, or at the very least make sure they are accurate. Especially with the removal of the derivatives attribute, I think there may be comments in lots of places that are no longer accurate. There are probably places I could add some comments.
  • Update the documentation. I haven't touched the docs documentation at all. I think there will need to be a lot of updating there. Off the top of my head I know the docs make reference to the "derivatives of variables" philosophy that is being replaced with the "linear combination of random variables" philosophy. I also know there will be some docs about copying/pickling that need to be updated, and of course docs on derivatives and error_components will need to be updated.
  • Bikeshed the naming of the UFloat, UCombo, and UAtom classes.
  • Write the changelog entries

Other notes:

  • COMMENT FOR REVIEWERS: Obviously I would appreciate wholistic reviews, But I'm especially curious for reviews on changes to the tests that start to illustrate how the public API is changing.
  • See this change. The to_uarray_func conversion function assumes inputs are numpy arrays and doesn't "respect" numpy matrices. I made this choice because of the comment on numpy.matrix documentation discouraging their use. See 4.x Release plans #244 (comment). It seems like the maintainers are in some agreement that we can drop matrix support, so maybe consider this a step in that direction.
  • I'll update this comment or later ones as I think of more changes or todos. I'll likely use this comment as a start for the changelog.

@newville
Copy link
Member
newville commented Jun 8, 2025

@jagerber48 Sorry, maybe I'm being dense or not following closely enough. Is this intended to be merged into master for the 3 series, or into main?
Either is OK with me. If master/3, is this ready for review?

@jagerber48 jagerber48 changed the base branch from master to main June 8, 2025 19:16
@jagerber48
Copy link
Contributor Author

@newville sorry, this PR originally targeted master but with our plan to release 4.x from main this PR should merge into main. I've changed the PR base just now.

I think merging this PR into main will be the big PR review before we release 4.0.0. This PR is not ready for review. It still needs

(1) to address the performance regression. Before I would say it had a major performance regression. Now the regression is only ~20%. At least on the one performance benchmark we have. See #327.
(2) The documentation needs to be rewritten taking into account that new changes that is what I'm working on presently
(3) During and after the above two items I will be going over things myself and I imagine there will continue to be small changes to the source code and tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewgsavage andrewgsavage andrewgsavage left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jagerber48 @newville @andrewgsavage
0