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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Vector(2|3|4)<T>::NaN to easily create invalid vectors #222

Merged
merged 6 commits into from
Sep 15, 2021

Conversation

chapulina
Copy link
Contributor

馃帀 New feature

Summary

I often need to create invalid vectors to indicate that something isn't initialized or became invalid. There are ways of expressing this with null pointers or std::nullopt, but using NaNs makes it convenient to do without involving the creation and destruction of new vectors.

This also plays well with the existing IsFinite and Correct functions. Note that none of this applies to integer types.

Test it

Take a look at the added tests.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@github-actions github-actions bot added 馃彚 edifice Ignition Edifice 馃彲 fortress Ignition Fortress 馃彴 citadel Ignition Citadel 馃敭 dome Ignition Dome labels Aug 18, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Aug 18, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Aug 18, 2021
@codecov
Copy link

codecov bot commented Aug 18, 2021

Codecov Report

Merging #222 (564de1c) into ign-math6 (82acdff) will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##           ign-math6     #222      +/-   ##
=============================================
+ Coverage      99.22%   99.39%   +0.17%     
=============================================
  Files             66       66              
  Lines           6162     6162              
=============================================
+ Hits            6114     6125      +11     
+ Misses            48       37      -11     
Impacted Files Coverage 螖
include/ignition/math/Vector2.hh 100.00% <酶> (酶)
include/ignition/math/Vector3.hh 98.84% <酶> (+3.07%) 猬嗭笍
include/ignition/math/Vector4.hh 95.57% <酶> (+1.10%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 82acdff...564de1c. Read the comment docs.

@chapulina chapulina requested a review from nkoenig August 23, 2021 18:42
@nkoenig
Copy link
Contributor

nkoenig commented Aug 30, 2021

@chapulina , do you want to try adding the nan types to the corresponding swig .i files?

@chapulina
Copy link
Contributor Author

do you want to try adding the nan types to the corresponding swig .i files?

Done in 1233218

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks good. Although I'd argue std::optional is more convenient than this for representing uninitialized values. For one, when checking if a variable is nan, you can't compare it against Vector::NaN directly. You'd have to use IsFinite, which I don't think would be immediately apparent to users. Also, IsFinite returns false if any of the members are nan, or inf, so it could potentially lead to bugs if Vector::NaN is used to represent uninitialized vectors.

include/ignition/math/Vector4.hh Outdated Show resolved Hide resolved
src/ruby/Vector2_TEST.rb Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

Thanks for the review, @azeey, I addressed your comments in 564de1c.

I also appreciate the feedback on the motivation for this PR. The specific use case that motivated this was a vector whose value keeps changing, and at times I actually needed to fill it with NaNs. But at other times I've also needed to "flag" that a vector's value was invalid temporarily, and I think it could be useful there as well, but I agree that there are multiple ways of going about this.

@chapulina chapulina enabled auto-merge (squash) September 15, 2021 23:41
@chapulina chapulina merged commit 0d884db into ign-math6 Sep 15, 2021
Core development automation moved this from In review to Done Sep 15, 2021
@chapulina chapulina deleted the chapulina/6/nan branch September 15, 2021 23:57
@chapulina chapulina moved this from Done to Highlights in Core development Sep 16, 2021
arjo129 pushed a commit that referenced this pull request Sep 20, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@j-rivero j-rivero removed this from Highlights in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 馃彴 citadel Ignition Citadel 馃敭 dome Ignition Dome 馃彚 edifice Ignition Edifice 馃彲 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants