Skip to content

Conversation

damskii9992
Copy link
Contributor

No description provided.

@damskii9992 damskii9992 added [scope] enhancement Adds/improves features (major.MINOR.patch) feature PR label labels Aug 1, 2024
@damskii9992 damskii9992 linked an issue Aug 1, 2024 that may be closed by this pull request
@rozyczko
Copy link
Member

rozyczko commented Aug 2, 2024

Apart from code review, a few things I found while testing

__radd__ vs. __add__ - should be consistent

    3+3+p -> "6+p"
    p+3+3 -> "p+3+3"

Nan/inf treatment. Are the p+np.inf bounds correct?

>>> p
<Parameter 'a': 1.0000, bounds=[-inf:inf]>
>>> p+np.inf
<Parameter 'a + inf': inf, bounds=[nan:inf]>
>>> p+np.nan
<Parameter 'a + nan': nan, bounds=[nan:nan]>

___div___ issue with the order (probably a scipp issue, or something wrong with __rdiv__ vs. __div__)

>>> a1
<Parameter 'a1': 2.0000 km, bounds=[-inf:inf]>
>>> a3
<Parameter 'a3': 2.0000 nm, bounds=[-inf:inf]>
>>> a1/a3
<Parameter 'a1 / a3': 1000000000000.0000, bounds=[-inf:inf]>
>>> a3/a1
scipp._scipp.core.UnitError: Conversion from `10e-13` to `2.18154854429043519e+244*s^(-13)*A^(-13)` is not valid.

@henrikjacobsenfys
Copy link
Member

b2=Parameter("b2", 2.0)
In [20]: b3=b2+1
In [21]: b3
Out[21]: <Parameter 'b2 + 1': 3.0000, bounds=[-inf:inf]>
In [22]: b3*2
Out[22]: <Parameter 'b2 + 1 * 2': 6.0000, bounds=[-inf:inf]>

Should probably be
<Parameter '(b2 + 1) * 2': 6.0000, bounds=[-inf:inf]>

@damskii9992 damskii9992 merged commit 2444f1b into develop Aug 14, 2024
@damskii9992 damskii9992 deleted the 27-Arithmetic-operations-for-Parameters branch August 14, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature PR label [scope] enhancement Adds/improves features (major.MINOR.patch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Arithmetic operations for Parameters and DescriptorNumbers

4 participants