Skip to content

Conversation

@sawenzel
Copy link
Contributor

  • a direct ApplyInverse() function for Points and Vectors
  • faster than using the Inverse() + operator() mechanism, because
    • avoids intermediate calculation and memory of inverse
    • we know the precise form of the inverse transformation a priori
      (the inverse of an 3D rotation is is transpose, etc.)

@phsft-bot
Copy link

Can one of the admins verify this patch?

@sawenzel
Copy link
Contributor Author

@lmoneta : Is there a unit test to which I can add a check for the new method. Please let me know what you think.

@vgvassilev
Copy link
Member

@phsft-bot build!

@sawenzel, this looks good. You can add a unit test under math//genvector/test/testGenVector.cxx and test/testGenVectorVc.cxx. The latter should probably be moved in the folder of the former.

About unit tests: #451 introduces a gtest in tree. It will land soon but I don't think this PR should wait for it.

@vgvassilev vgvassilev self-assigned this Mar 27, 2017
return operator()(v);
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

It seems we are missing some punctuation here.

}

/**
Directly apply the inverse affine transformation on Points
Copy link
Member

Choose a reason for hiding this comment

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

I'd add punctuation here, too.

fM[kXZ] * tmp.X() + fM[kYZ] * tmp.Y() + fM[kZZ] * tmp.Z());
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

I'd for a complete sentence starting with capital letter.

return PositionVector3D<CoordSystem>(ApplyInverse(Point(p)));
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

I'd form a sentence here, too.

@sawenzel
Copy link
Contributor Author

@vgvassilev : Thanks for the review. Will fix the comments.

@lmoneta
Copy link
Member

lmoneta commented Mar 28, 2017

Sandro, thank you for the PR !

Yes, It would be great to have also a unit test in the math/genvector/test/testGenVector.cxx test program

Best

Lorenzo

@sawenzel sawenzel force-pushed the swenzel/Transform3D branch from a4f97b8 to 119a85f Compare March 28, 2017 14:55
@sawenzel
Copy link
Contributor Author

I added a unit test and fixed the spelling things.

@vgvassilev
Copy link
Member

Thanks! It seems clang-format is unhappy.

@sawenzel
Copy link
Contributor Author

Ok. Will check.

@sawenzel
Copy link
Contributor Author

sawenzel commented Mar 28, 2017

@vgvassilev : Which file? If I apply clang-format on the unit test ... the whole file is changed (not just my part). Same for Transform3D

@sawenzel sawenzel force-pushed the swenzel/Transform3D branch from 119a85f to 37cef30 Compare March 28, 2017 15:12
@vgvassilev
Copy link
Member

vgvassilev commented Mar 28, 2017

@sawenzel, you can get the diff (by clicking on view raw log) and apply it or run git-clang-format --commit 473bb4504c382d706deeeb96ea666dfa61222baa --binary ${which clang-format}.

@sawenzel sawenzel force-pushed the swenzel/Transform3D branch 3 times, most recently from 1d771f3 to b57d8ae Compare March 28, 2017 15:21
…rm3D)

 * a direct ApplyInverse() function for Points and Vectors
 * faster than using the Inverse() + operator() mechanism, because
   - avoid intermediate calculation and memory of inverse
   - we know the precise form of the inverse transformation a priori
     (the inverse of an 3D rotation is is transpose, etc.)
 * added unit test
@sawenzel sawenzel force-pushed the swenzel/Transform3D branch from b57d8ae to aaf4f79 Compare March 28, 2017 15:23
@vgvassilev
Copy link
Member

@phsft-bot build!

@phsft-bot
Copy link

Starting build on centos7/gcc49, mac1011/native, slc6/gcc49, slc6/gcc62, ubuntu14/native

@vgvassilev
Copy link
Member

@martinmine, "centos7/gcc49, mac1011/native, slc6/gcc49, slc6/gcc62, ubuntu14/native": Could we change one of the default platforms from gcc49 to gcc52. This way we would have better compiler coverage...

@vgvassilev
Copy link
Member

It seems the failures are unrelated to this PR. Merging. Thanks for the contribution!

@vgvassilev vgvassilev merged commit cc8b2b2 into root-project:master Mar 28, 2017
gganis pushed a commit to gganis/root that referenced this pull request Apr 3, 2017
…rm3D) (root-project#464)

* a direct ApplyInverse() function for Points and Vectors
 * faster than using the Inverse() + operator() mechanism, because
   - avoid intermediate calculation and memory of inverse
   - we know the precise form of the inverse transformation a priori
     (the inverse of an 3D rotation is is transpose, etc.)
 * added unit test
amadio pushed a commit to amadio/root that referenced this pull request May 23, 2018
…rm3D) (root-project#464)

* a direct ApplyInverse() function for Points and Vectors
 * faster than using the Inverse() + operator() mechanism, because
   - avoid intermediate calculation and memory of inverse
   - we know the precise form of the inverse transformation a priori
     (the inverse of an 3D rotation is is transpose, etc.)
 * added unit test
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.

4 participants