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

time.h durationToSec seems very inefficient #34

Closed
nate-jackson opened this issue Jul 13, 2017 · 6 comments
Closed

time.h durationToSec seems very inefficient #34

nate-jackson opened this issue Jul 13, 2017 · 6 comments

Comments

@nate-jackson
Copy link

We have been investigating why we can't seem to get lookupTransform to work properly for us, and came across this function.

Instead of doing the big long thing that is going on there:

inline double durationToSec(const tf2::Duration& input){
    int64_t count = input.count();
    int32_t sec, nsec;
    nsec = static_cast<int32_t>(count % 1000000000l);
    sec = static_cast<int32_t>((count - nsec) / 1000000000l);

    double sec_double, nsec_double;
    nsec_double = 1e-9 * static_cast<double>(nsec);
    sec_double = static_cast<double>(sec);
    return sec_double + nsec_double;
  }

couldn't we just do

inline double durationToSec(const tf2::Duration& input){
    return input.count() * 1e-9;
  }

I can't see a reason for the extra stuff that is going on in there.

@wjwwood
Copy link
Member

wjwwood commented Jul 13, 2017

Sounds reasonable to me, it's probably left over from the conversion. @dhood what do you think?

@dhood
Copy link
Member

dhood commented Jul 14, 2017

the proposed conversion isn't as accurate. We might still prefer it (since the conversion isn't exact as is), but in that case we'd need to relax the tolerance on the tests (e.g. this one) and disable this test

@dhood
Copy link
Member

dhood commented Jul 14, 2017

Here's CI to show what I mean (these tests were run with --retest-until-fail 20 because the reduced accuracy would manifest as a flaky test since it wouldn't fail for all values of 'now'):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Jul 14, 2017

Ah, @dhood I forgot about that. Perhaps we should put a comment explaining why the nanoseconds are scaled separately.

@dhood
Copy link
Member

dhood commented Jul 14, 2017

sure thing: #35

@dhood
Copy link
Member

dhood commented Jul 14, 2017

I'll close this because I believe the question has been addressed. if that's not the case feel free to re-open

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

No branches or pull requests

3 participants