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

Add Max and Min function to Vector2.hh #133

Merged
merged 9 commits into from
Aug 10, 2020

Conversation

pxalcantara
Copy link
Contributor

Solved issue #71

Functions added to solved the issue mentioned above, there was implemented:

  • Max(const Vector2 &_v);
  • Min(const Vector2 &_v);
  • Max();
  • Min();

These functions were based on the functions with the same name at Vector3.hh

@chapulina chapulina added enhancement New feature or request 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Jul 15, 2020
include/ignition/math/Vector2.hh Outdated Show resolved Hide resolved
include/ignition/math/Vector2.hh Outdated Show resolved Hide resolved
include/ignition/math/Vector2.hh Outdated Show resolved Hide resolved
@pxalcantara
Copy link
Contributor Author

pxalcantara commented Jul 18, 2020

@mjcarroll should I add these functions to the Vector2.i file, as did in #137 ?

@mjcarroll
Copy link
Contributor

@mjcarroll should I add these functions to the Vector2.i file, as did in #137 ?

Yes please, I'll leave it up to you if you want to do it here or in a follow-up.

@pxalcantara
Copy link
Contributor Author

Yes please, I'll leave it up to you if you want to do it here or in a follow-up.

I'll do it here. By the way, once the PR #123 was merged, I need to update my branch to continue with this PR, right?

Thanks!

@pxalcantara
Copy link
Contributor Author

@chapulina Did you deleted some comment? I received an e-mail with your comment but I can't see it here 😕

@chapulina
Copy link
Contributor

@chapulina Did you deleted some comment?

haha yeah I did, I thought I was commenting on #139 but I had the wrong tab open. I was hoping no one had seen it 😅

@mjcarroll
Copy link
Contributor

I need to update my branch to continue with this PR, right?

Yes, you can either merge ign-math6 into your branch, or rebase your branch onto ign-math6

@mjcarroll
Copy link
Contributor

@pxalcantara friendly ping

@pxalcantara
Copy link
Contributor Author

@pxalcantara friendly ping

Sorry @mjcarroll I've been a little busy, but I gonna finish this today 🙈

@pxalcantara
Copy link
Contributor Author

@mjcarroll done, again, sorry for the delay! 😉

@pxalcantara
Copy link
Contributor Author

pxalcantara commented Jul 28, 2020

@mjcarroll I forgot to add the tests to .rb file. How I run just the Vector2_TEST.rb to check if the test that I'm doing is correct?

@mjcarroll
Copy link
Contributor

How I run just the Vector2_TEST.rb to check if the test that I'm doing is correct?

You should be able to go to the build directory and filter it with ctest

cd build
ctest -R Vector2_TEST.rb

@pxalcantara
Copy link
Contributor Author

How I run just the Vector2_TEST.rb to check if the test that I'm doing is correct?

You should be able to go to the build directory and filter it with ctest

cd build
ctest -R Vector2_TEST.rb

@luccosta Maybe worth add this information at the documentation that you're doing about the Ruby tests

@luccosta
Copy link
Contributor

@luccosta Maybe worth add this information at the documentation that you're doing about the Ruby tests

It's already there :) but without cd build, I will add.

@pxalcantara
Copy link
Contributor Author

@mjcarroll I'm facing some problems doing the test at Vector2.br

overview@px:ignition-math6$ ctest -R Vector2_TEST.rb -V
UpdateCTestConfiguration  from :/home/overview/citadel_ws/build/ignition-math6/DartConfiguration.tcl
Parse Config file:/home/overview/citadel_ws/build/ignition-math6/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/overview/citadel_ws/build/ignition-math6/DartConfiguration.tcl
Parse Config file:/home/overview/citadel_ws/build/ignition-math6/DartConfiguration.tcl
Test project /home/overview/citadel_ws/build/ignition-math6
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 85
    Start 85: Vector2_TEST.rb

85: Test command: /usr/bin/ruby "-I/home/overview/citadel_ws/build/ignition-math6/lib" "/home/overview/citadel_ws/src/ign-math/src/Vector2_TEST.rb" "--gtest_output=xml:/home/overview/citadel_ws/build/ignition-math6/test_results/Vector2_TESTrb.xml"
85: Test timeout computed to be: 1500
85: Loaded suite Vector2_TEST
85: Started
85: ......E
85: ===============================================================================
85: Error: test_max(Vector2_TEST): NoMethodError: undefined method `Max' for #<Ignition::Math::Vector2d:0x000055c8df42ed40>
85: /home/overview/citadel_ws/src/ign-math/src/Vector2_TEST.rb:116:in `test_max'
85:      113:      vec2 = Ignition::Math::Vector2d.new(0.3, 0.5)
85:      114:      vec3 = Ignition::Math::Vector2d.new(0.4, 0.2)
85:      115:      
85:   => 116:      assert((vec1.Max() - 0.2).abs() < 1e-10,
85:      117:            "Vector3 vec1.Max should equal 0.3")
85:      118:      
85:      119:      vec1.Max(vec2)
85: ===============================================================================
85: ..
85: 
85: Finished in 0.004180697 seconds.
85: ------
85: 9 tests, 48 assertions, 0 failures, 1 errors, 0 pendings, 0 omissions, 0 notifications
85: 88.8889% passed
85: ------
85: 2152.75 tests/s, 11481.34 assertions/s
1/1 Test #85: Vector2_TEST.rb ..................***Failed    0.17 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) =   0.18 sec

The following tests FAILED:
	 85 - Vector2_TEST.rb (Failed)
Errors while running CTest

I've already added these functions to Vector2.i. Could you help me and show what I'm doing wrong?
Thanks!

@mjcarroll
Copy link
Contributor

Could you help me and show what I'm doing wrong?

Yep!

First a note that the three ignition_math-ci jobs pass because we don't do the Ruby bindings in them.

When I check out your branch, everything looks to pass for me locally:

85: ------
85: 9 tests, 51 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
85: 100% passed
85: ------

Maybe once #139 lands, we can rebase this and see how it looks in Github Actions? There are obviously inconsistencies between our two systems if we are getting different results like this.

@pxalcantara
Copy link
Contributor Author

So, as far as I understood, my code is correct, so I can continue to implement the test in the Ruby file, right? @mjcarroll @chapulina

@mjcarroll
Copy link
Contributor

Yes, it currently works fine for me, feel free to continue @pxalcantara

@pxalcantara
Copy link
Contributor Author

@mjcarroll @chapulina Apparently there is a problem with my system because this test continuos to break but, once it passed the CI check here, that's what mater. Sorry for the PR take so long but I think now is finished, right? Or do you wanna me to do more changes?

@mjcarroll
Copy link
Contributor

Passing CI is sufficient for me!

@mjcarroll mjcarroll merged commit 6e80a49 into gazebosim:ign-math6 Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants