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

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

Merged

Conversation

Crola1702
Copy link
Contributor

@Crola1702 Crola1702 commented Sep 1, 2022

🪰 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

Signed-off-by: Crola1702 <j.arroyo@uniandes.edu.co>
Signed-off-by: Crola1702 <j.arroyo@uniandes.edu.co>
Signed-off-by: Crola1702 <j.arroyo@uniandes.edu.co>
Signed-off-by: Crola1702 <j.arroyo@uniandes.edu.co>
Signed-off-by: Crola1702 <j.arroyo@uniandes.edu.co>
Copy link

@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

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
Copy link
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
@fujitatomoya
Copy link
Contributor

@mergify backport humble

@mergify
Copy link

mergify bot commented Nov 5, 2022

backport humble

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

  • sender-permission>=write

@fujitatomoya
Copy link
Contributor

@Mergifyio backport humble

@mergify
Copy link

mergify bot commented Nov 5, 2022

backport humble

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

  • sender-permission>=write

Copy link
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
* Replaced equal_nan for numpy<1.19

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
clalancette added a commit that referenced this pull request Nov 8, 2022
* 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 <os@skarvtech.com>

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

Signed-off-by: Øystein Sture <os@skarvtech.com>

* Add float/double NaN/Inf tests

Signed-off-by: Øystein Sture <os@skarvtech.com>

* Increase precision of float32 bounds

Signed-off-by: Øystein Sture <os@skarvtech.com>

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

Signed-off-by: Øystein Sture <os@skarvtech.com>

* Disable bounds test for float64 on IEEE 754 compatible systems

Signed-off-by: Øystein Sture <os@skarvtech.com>

* Disable bounds test for float64 on IEEE 754 compatible systems

Signed-off-by: Øystein Sture <os@skarvtech.com>

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

Signed-off-by: Øystein Sture <os@skarvtech.com>

* Change decode error mode to replace (#176)

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* Replace rosidl_cmake imports with rosidl_pycommon (#177)

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Delete trailing whitespace

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Split fail case array check tests into two

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Øystein Sture <os@skarvtech.com>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>

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

* Replaced equal_nan for numpy<1.19

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>

* 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 <benwolsieffer@gmail.com>

Signed-off-by: Øystein Sture <os@skarvtech.com>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
Co-authored-by: Øystein Sture <oysstu@users.noreply.github.com>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
Co-authored-by: Cristóbal Arroyo <69475004+Crola1702@users.noreply.github.com>
Co-authored-by: Ben Wolsieffer <benwolsieffer@gmail.com>
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.

None yet

4 participants