split core into smaller files #237

andrewgsavage · 2024-06-04T22:10:56Z

the changes from #198

codecov · 2024-06-04T22:13:04Z

Codecov Report

Attention: Patch coverage is 96.27451% with 19 lines in your changes missing coverage. Please review.

Project coverage is 95.79%. Comparing base (1238d37) to head (5d45d94).

Files	Patch %	Lines
uncertainties/parsing.py	84.61%	10 Missing ⚠️
uncertainties/ops.py	96.78%	7 Missing ⚠️
uncertainties/formatting.py	99.02%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   95.73%   95.79%   +0.05%     
==========================================
  Files          12       15       +3     
  Lines        1901     1927      +26     
==========================================
+ Hits         1820     1846      +26     
  Misses         81       81

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

andrewgsavage · 2024-06-08T08:05:02Z

@lmfit/uncertainties-admins This is ready for review now.

This is mostly copy and paste, with some changes to prevent passing AffineScalarFunc and LinearCombination to the arithmetic/comparative op functions before they're defined.

AffineScalarFunc becomes cls and is used in a function that will be called on AffineScalarFunc in core.py
AffineScalarFunc(nominal_value, linear_part) will now call LinearCombination on linear_part if it's not already a LinearCombination

newville · 2024-07-04T13:10:19Z

@andrewgsavage sorry for the delay on this. I think this splitting up of the code is very helpful.

+1 on merging.

newville

This looks great (and, again, sorry for the delay). I might have kept "ops" and "parsing" together, but this is perfectly reasonable too.

I think the complaints from codecov are noise we can ignore here, and maybe try to address separately.

wshanks · 2024-07-04T15:00:30Z

I also like this refactoring in principle. I had wanted to find time to go through and follow how things were moving around in the namespace, but I am fine with @newville's approval.

One thing to note: a grep of the repo for uncertainties.core reveals references to uncertainties.core.format_num and uncertainties.core.MULT_SYMBOLS in the documentation. format_num is imported from uncertainties.formatting to uncertainties.core here, so its usage should still work. MULT_SYMBOLS is not imported currently.

So I would consider the following:

uncertainties/__init__.py currently has a from uncertainties.core import *. Is there anything else users could be using in the uncertainties namespace that was moved out of uncertainties.core here?
Should the documentation be updated to use uncertainties.formatting.format_num (or just uncertainties.format_num given the point above about from uncertainties import *)?
Should MULT_SYMBOLS be imported into uncertainties.core for backwards compatibility?

Outside of this PR, I think it would be good to get rid of the from ... import * and also to use __all__ to be explicit about which symbols are public and safe for a user to access.

andrewgsavage · 2024-07-05T07:21:58Z

uncertainties/__init__.py currently has a from uncertainties.core import *. Is there anything else users could be using in the uncertainties namespace that was moved out of uncertainties.core here?

good question, I don't see any other ways to approach this other than what's in __all__ or the docs grep you did.

Should the documentation be updated to use uncertainties.formatting.format_num (or just uncertainties.format_num given the point above about from uncertainties import *)?

It's not yet clear to me what I'd like uncertainties namespace to look like so I'd hold off on changing docs until we're happy with where things are located.

For example I think it'd be worthwhile moving the code used to create the classes Variable, LinearCombination, AffineScalarFunc
away from the functions that operate or return them, eg ufloat, covariance_matrix

Should MULT_SYMBOLS be imported into uncertainties.core for backwards compatibility?

Done

Outside of this PR, I think it would be good to get rid of the from ... import * and also to use __all__ to be explicit about which symbols are public and safe for a user to access.

Absolutely, atm I'm implicitly taking it to be anything defined in __all__ or in the docs.

andrewgsavage · 2024-07-07T20:03:21Z

can this be merged?

newville

This is a big change, and may not be completely finalized. I think @wshanks had good points, but I think we should merge this and move forward from this branch. Do you agree @wshanks?

wshanks

I found some time to look at this more closely. I think it looks good overall, but I noticed a few small things that I commented on in line.

I realized that my previous comment about __all__ was not completely accurate. There already is an __all__ in core.py that gets pulled up into uncertainties/__init__.py. We should try to promote the two things in uncertainties.core referenced in the documentation to uncertainties and uncertainties.__all__ and then discourage use of anything not in uncertainties.__all__. We don't have to do that here. I checked that this PR does remove anything that was already in uncertainties or uncertainties.__all__.

One side comment -- I am not sure I like the way this PR moves some of the logic of setting up AffineScalarFunc out of core.py by moving it into functions of cls in other files where cls is intended to be only AffineScalarFunc so that those functions can be defined before AffineScalarFunc so they can be imported into core.py and called on AffineScalarFunc.

wshanks · 2024-07-08T21:13:38Z

uncertainties/core.py

 # Nicer name, for users: isinstance(ufloat(...), UFloat) is
 # True. Also: isinstance(..., UFloat) is the test for "is this a
 # number with uncertainties from the uncertainties package?":
 UFloat = AffineScalarFunc

+def wrap(*args, **kwargs):


wrap is a public function and this change loses the docstring and signature that it currently has. It would be better to keep those here.

wshanks · 2024-07-08T21:22:46Z

uncertainties/formatting.py

+
+
+def isinfinite(x):
+    # TODO: Usages of this function should be replaced with not math.isfinite.


This came from a code block that was providing backwards compatibility for Python before 3.2. It could just be from math import isinfinite now.

wshanks · 2024-07-08T21:31:38Z

uncertainties/core.py

-    def format(*args, **kwargs):
-        return args[0].__format__(*args[1:], **kwargs)
-
+        return format_ufloat(self, format_spec)


This lost a big docstring (now in format_ufloat). The __format__ docstring is currently referenced by the format_num docstring.

newville · 2024-07-08T22:04:15Z

@wshanks Thanks. From a practical point of view, do you think you and @andrewgsavage should work these things out before merging, or do you think we can merge this work and then fix this once in master?

I don't have a strong opinion on this. Having just released "solid, patched 3.2.2", I am okay with a master branch that may be a little messy and may be revised as we move toward 4.0.

I suggest you merge this when you think it is close enough, and we can work from that point.

andrewgsavage · 2024-07-08T23:28:53Z

One side comment -- I am not sure I like the way this PR moves some of the logic of setting up AffineScalarFunc out of core.py by moving it into functions of cls in other files where cls is intended to be only AffineScalarFunc so that those functions can be defined before AffineScalarFunc so they can be imported into core.py and called on AffineScalarFunc.

Is there a better way to do this?
Could have a class in ops that defines the logic , which AffineScalarFunc inherits from. Like how pandas does so
https://github.com/pandas-dev/pandas/blob/ab433af410464f4f5c377c82a3d4f5680bf3c65c/pandas/core/arrays/base.py#L2457

wshanks · 2024-07-09T00:23:07Z

From a practical point of view, do you think you and @andrewgsavage should work these things out before merging, or do you think we can merge this work and then fix this once in master?

The three comments I put in line should be easy to address, I think. The other comments I put in the summary don't need to be addressed here.

Is there a better way to do this?

I have to think about it more. It might be something that is easier after more refactoring but we don't need to change it here.

andrewgsavage · 2024-07-09T19:15:51Z

ok addressed those

wshanks · 2024-07-10T15:11:28Z

Looks good!

andrewgsavage changed the title ~~move formatting to new file~~ split core into smaller files Jun 5, 2024

newville previously approved these changes Jul 4, 2024

View reviewed changes

andrewgsavage dismissed newville’s stale review via a38e017 July 5, 2024 06:47

andrewgsavage added 5 commits July 7, 2024 17:47

move formatting to new file

8212ece

split ops out

a2cb45e

split out parsing

8c77952

comparatives

77a1f8d

multi symbols

b3e47f4

newville force-pushed the formatting branch from a38e017 to b3e47f4 Compare July 7, 2024 22:47

newville previously approved these changes Jul 7, 2024

View reviewed changes

wshanks reviewed Jul 8, 2024

View reviewed changes

wraps

58a80b9

andrewgsavage dismissed newville’s stale review via 58a80b9 July 9, 2024 19:06

andrewgsavage added 2 commits July 9, 2024 20:08

docstring

6089a3a

docstring

5d45d94

wshanks approved these changes Jul 10, 2024

View reviewed changes

wshanks merged commit 3d2b16d into lmfit:master Jul 10, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split core into smaller files #237

split core into smaller files #237

andrewgsavage commented Jun 4, 2024

codecov bot commented Jun 4, 2024 •

edited

Loading

andrewgsavage commented Jun 8, 2024

newville commented Jul 4, 2024

newville left a comment

wshanks commented Jul 4, 2024

andrewgsavage commented Jul 5, 2024

andrewgsavage commented Jul 7, 2024

newville left a comment

wshanks left a comment

wshanks Jul 8, 2024

wshanks Jul 8, 2024

wshanks Jul 8, 2024

newville commented Jul 8, 2024

andrewgsavage commented Jul 8, 2024

wshanks commented Jul 9, 2024

andrewgsavage commented Jul 9, 2024

wshanks commented Jul 10, 2024



		def isinfinite(x):
		# TODO: Usages of this function should be replaced with not math.isfinite.

split core into smaller files #237

split core into smaller files #237

Conversation

andrewgsavage commented Jun 4, 2024

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

andrewgsavage commented Jun 8, 2024

newville commented Jul 4, 2024

newville left a comment

Choose a reason for hiding this comment

wshanks commented Jul 4, 2024

andrewgsavage commented Jul 5, 2024

andrewgsavage commented Jul 7, 2024

newville left a comment

Choose a reason for hiding this comment

wshanks left a comment

Choose a reason for hiding this comment

wshanks Jul 8, 2024

Choose a reason for hiding this comment

wshanks Jul 8, 2024

Choose a reason for hiding this comment

wshanks Jul 8, 2024

Choose a reason for hiding this comment

newville commented Jul 8, 2024

andrewgsavage commented Jul 8, 2024

wshanks commented Jul 9, 2024

andrewgsavage commented Jul 9, 2024

wshanks commented Jul 10, 2024

codecov bot commented Jun 4, 2024 •

edited

Loading