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

Set default _numOutputs to 1 in splitEngine #214

Merged
merged 2 commits into from
Jun 26, 2017

Conversation

brianhou
Copy link
Contributor

@brianhou brianhou commented Jun 1, 2017

Resolves #187.

@mkoval is this what you meant? My initial thought was to have the two methods:

  • splitEngine(RNG& _engine, size_t _numOutputs, size_t _numSeeds = NUM_DEFAULT_SEEDS) (already exists)
  • splitEngine(RNG& _engine, size_t _numSeeds = NUM_DEFAULT_SEEDS) (call the former with _numOutputs = 1)

but this makes splitEngine(RNG&, size_t) ambiguous.

@brianhou brianhou requested a review from jslee02 June 1, 2017 17:13
@codecov
Copy link

codecov bot commented Jun 1, 2017

Codecov Report

Merging #214 into master will decrease coverage by 7.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
- Coverage   72.03%   64.88%   -7.15%     
==========================================
  Files         157      156       -1     
  Lines        4487     4443      -44     
  Branches      707      703       -4     
==========================================
- Hits         3232     2883     -349     
- Misses        793     1164     +371     
+ Partials      462      396      -66
Impacted Files Coverage Δ
include/aikido/util/RNG.hpp 100% <ø> (ø) ⬆️
src/util/RNG.cpp 92.3% <100%> (ø) ⬆️
src/constraint/JointStateSpaceHelpers.cpp 70.83% <100%> (ø) ⬆️
include/aikido/planner/ompl/CRRT.hpp 0% <0%> (-100%) ⬇️
...lude/aikido/planner/ompl/BackwardCompatibility.hpp 25% <0%> (-75%) ⬇️
src/planner/ompl/CRRTConnect.cpp 0% <0%> (-64.03%) ⬇️
src/planner/ompl/CRRT.cpp 0% <0%> (-63.96%) ⬇️
src/planner/ompl/Planner.cpp 7.3% <0%> (-50.57%) ⬇️
src/planner/ompl/MotionValidator.cpp 5.4% <0%> (-24.33%) ⬇️
src/planner/ompl/StateValidityChecker.cpp 15.38% <0%> (-23.08%) ⬇️
... and 4 more

@jslee02
Copy link
Member

jslee02 commented Jun 1, 2017

According to the description of #187, it seems @brianhou 's initial thought is what @mkoval means. To avoid the function name's ambiguity, we could use different name. If the intention is to create one object with the same type of another existing object, I would like to call the function clone (or cloneFrom).

Also, we could use cloneRNGs for multiple cloning version instead of splitEngine.

I'm open to using different name from what I proposed, but my point is to use different function name to avoid the ambiguity.

Or, we could remove all the default parameters of the original function.

@brianhou
Copy link
Contributor Author

brianhou commented Jun 1, 2017

Different function names sound reasonable, although I think it'd make more sense to keep the names similar. How about cloneRngFrom (for the new one-output version) and cloneRngsFrom (for the current splitEngine)?

I'd prefer to keep the default parameters; I don't want to arbitrarily choose a value for _numSeeds each time I use it 😅

@jslee02
Copy link
Member

jslee02 commented Jun 1, 2017

cloneRngFrom and cloneRngsFrom overall sound good to me, but have a minor concern that they use different capitalization (RNG vs Rng). I personally prefer Rng for any other acronyms, but it seems the current convention is RNG. So we may want to stay with it like cloneRNGFrom and cloneRNGsFrom unless we change the convention and apply it to all other names.

Also, out of curiosity, do we have a specific reason why not we have this functions as member functions?

@brianhou brianhou self-assigned this Jun 12, 2017
@brianhou brianhou merged commit cdee7c9 into master Jun 26, 2017
@brianhou brianhou deleted the enhancement/brianhou/splitEngine branch June 26, 2017 18:59
@jslee02 jslee02 added this to the Aikido 0.1.0 milestone Sep 26, 2017
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.

2 participants