Skip to content

👨‍🌾 Fix NaN values bound numpy windows version#182

Merged
Crola1702 merged 5 commits intorollingfrom
Crola1702/fix-nan-values-bound-numpy-windows-version
Sep 2, 2022
Merged

👨‍🌾 Fix NaN values bound numpy windows version#182
Crola1702 merged 5 commits intorollingfrom
Crola1702/fix-nan-values-bound-numpy-windows-version

Conversation

@Crola1702
Copy link
Copy Markdown
Contributor

@Crola1702 Crola1702 commented Sep 1, 2022 •
edited
Loading

🪰 BugFix

Issue

Reference build: Nightly Windows Debug

Log output:

>       assert numpy.array_equal(arr_of_float32_with_nan, msg.float32_values, equal_nan=True)

C:\ci\ws\src\ros2\rosidl_python\rosidl_generator_py\test\test_interfaces.py:508: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = (array([-1.33,   nan,  1.33], dtype=float32), array([-1.33,   nan,  1.33], dtype=float32))
kwargs = {'equal_nan': True}

>   ???
E   TypeError: _array_equal_dispatcher() got an unexpected keyword argument 'equal_nan'

<__array_function__ internals>:4: TypeError

Investigation

The proposed solution changes this test behavior to support both numpy <= 1.18 and >= 1.19.

Selected implementation: https://github.com/rossbar/numpy/blob/main/numpy/core/numeric.py#L2475

asarray(a1[~a1nan] == a2[~a1nan]).all() # plus shape verification

Windows build: Build Status
Linux aarch64 build:Build Status
Linux build: Build Status

@Crola1702 Crola1702 requested a review from Blast545 September 1, 2022 17:48
Blast545
Blast545 approved these changes Sep 2, 2022
View reviewed changes
Copy link
Copy Markdown

@Blast545 Blast545 left a comment

Choose a reason for hiding this comment

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

LGTM

@Blast545
Copy link
Copy Markdown

Blast545 commented Sep 2, 2022

Do you think this approach is OK? @sloretz (tagging you as reviewer of the original PR)

@Crola1702 Crola1702 marked this pull request as ready for review September 2, 2022 14:11
clalancette
clalancette approved these changes Sep 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Seems reasonable enough to me.

The Rpr job is currently expected to fail here, so we can ignore that and just use the CI results.

@Crola1702 Crola1702 merged commit ddb5ee8 into rolling Sep 2, 2022
@delete-merged-branch delete-merged-branch bot deleted the Crola1702/fix-nan-values-bound-numpy-windows-version branch September 2, 2022 14:18
@sloretz sloretz mentioned this pull request Sep 21, 2022
Merged
@fujitatomoya fujitatomoya mentioned this pull request Nov 4, 2022
Closed
@fujitatomoya
Copy link
Copy Markdown
Contributor

fujitatomoya commented Nov 5, 2022

@mergify backport humble

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 5, 2022

backport humble

❌ Command disallowed due to command restrictions in the Mergify configuration.

Details
  • sender-permission>=write

@fujitatomoya
Copy link
Copy Markdown
Contributor

fujitatomoya commented Nov 5, 2022

@Mergifyio backport humble

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 5, 2022

backport humble

❌ Command disallowed due to command restrictions in the Mergify configuration.

Details
  • sender-permission>=write

fujitatomoya
fujitatomoya reviewed Nov 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@clalancette @sloretz this one too, what do you think?

clalancette pushed a commit that referenced this pull request Nov 7, 2022
@Crola1702 @clalancette
👨‍🌾 Fix NaN values bound numpy windows version (#182)
0172e75 
* Replaced equal_nan for numpy<1.19

Signed-off-by: Crola1702 <[email protected]>
@clalancette clalancette mentioned this pull request Nov 7, 2022
Merged
clalancette added a commit that referenced this pull request Nov 8, 2022
@clalancette @oysstu
@fujitatomoya @jacobperron @sloretz @Crola1702 @lopsided98
Humble: Backport NaN fixes (#188)
c881598 
* Allow NaN values to pass floating point bounds check. (#167)

* Invert bounds check in order to permit floating point NaN values

Signed-off-by: Øystein Sture <[email protected]>

* Allow floating point numbers set to Inf to pass bounds check

Signed-off-by: Øystein Sture <[email protected]>

* Add float/double NaN/Inf tests

Signed-off-by: Øystein Sture <[email protected]>

* Increase precision of float32 bounds

Signed-off-by: Øystein Sture <[email protected]>

* Only perform float64 bounds check on non-compliant IEEE 754 systems

Signed-off-by: Øystein Sture <[email protected]>

* Disable bounds test for float64 on IEEE 754 compatible systems

Signed-off-by: Øystein Sture <[email protected]>

* Disable bounds test for float64 on IEEE 754 compatible systems

Signed-off-by: Øystein Sture <[email protected]>

* Resolve failing nan/inf test by comparing the same types

Signed-off-by: Øystein Sture <[email protected]>

* Change decode error mode to replace (#176)

Signed-off-by: Tomoya Fujita <[email protected]>

* Replace rosidl_cmake imports with rosidl_pycommon (#177)

Signed-off-by: Jacob Perron <[email protected]>

* Delete trailing whitespace

Signed-off-by: Shane Loretz <[email protected]>

* Split fail case array check tests into two

Signed-off-by: Shane Loretz <[email protected]>

Signed-off-by: Øystein Sture <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
Co-authored-by: Jacob Perron <[email protected]>
Co-authored-by: Shane Loretz <[email protected]>

* 👨‍🌾 Fix NaN values bound numpy windows version (#182)

* Replaced equal_nan for numpy<1.19

Signed-off-by: Crola1702 <[email protected]>

* Remove stray numpy import (#185)

NumPy was being imported in the template code (not the template output) for
apparently no reason. This was causing problems when cross-compiling, where
NumPy is only available for the target platform.

Signed-off-by: Ben Wolsieffer <[email protected]>

Signed-off-by: Øystein Sture <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Crola1702 <[email protected]>
Signed-off-by: Ben Wolsieffer <[email protected]>
Co-authored-by: Øystein Sture <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
Co-authored-by: Jacob Perron <[email protected]>
Co-authored-by: Shane Loretz <[email protected]>
Co-authored-by: Cristóbal Arroyo <[email protected]>
Co-authored-by: Ben Wolsieffer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@clalancette clalancette clalancette approved these changes

@Blast545 Blast545 Blast545 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Crola1702 @Blast545 @fujitatomoya @clalancette