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

Initial integration of new xrd-transforms package #509

Merged
merged 11 commits into from
Apr 18, 2023
Merged

Conversation

psavery
Copy link
Collaborator

@psavery psavery commented Apr 4, 2023

This takes @ovillellas's xrd-transforms package and integrates it into hexrd.

hexrd is now building and using functions from both the old transforms package and the new transforms package. I think this is okay to do for some time for a couple of reasons:

  1. We want to test every function that we integrate very thoroughly to make sure that the numbers do not change, and that no code is broken. This is easier to do if we migrate over a few functions at a time and wait for feedback from users.
  2. This allows us to easily switch back to the old functions if we have any need to in the time being.

We can keep migrating some functions and testing them until we have all of them in. Once we do, and once it has been tested thoroughly, we can eliminate the old xfcapi code. But until then, we should proceed carefully.

That said, the first few functions that were migrated over were the ones found to be most important through profiling fit-grains here. The functions migrated over were: anglesToGVec, makeOscillRotMat, makeOscillRotMatArray, gvecToDetectorXY, and gvecToDetectorXYArray. Here are the changes:

  • anglesToGVec -> angles_to_gvec (arguments are all snake_case now)
  • makeOscillRotMat -> make_sample_rmat (also changed - keep chi and ome as separate arrays rather than concatenating them into the first argument)
  • makeOscillRotMatArray -> make_sample_rmat
  • gvecToDetectorXY -> gvec_to_xy (arguments are all snake_case now)
  • gvecToDetectorXYArray -> gvec_to_xy (arguments are all snake_case now)

You can see that makeOscillRotMat and makeOscillRotMatArray were combined into a single make_sample_rmat. This automatically figures out which underlying c function should be called based upon the arguments it was provided. Similarly, gvecToDetectorXY and gvecToDetectorXYArray are combined into gvec_to_xy, and the underlying c function is also chosen based upon the arguments provided.

The convention here is to keep the older function names in the camelCase syntax, and make the new function names be in the more Pythonic snake_case syntax. This helps us keep track of what has been migrated and what has not.

For fit-grains, these changes resulted in a 23% speed increase for running the the monolithic ruby example serially.

However, there is one important thing to note: the output from gvecToDetectorXY* is now slightly different. The max difference I saw was ~8e-13. But this difference accumulated/propagated over several calls so that the fit-grains output (grains.out) saw a difference in its tvec around 1e-5. All of the other values were different as well (except for completeness - which was an exact match), and the other differences were between 1e-6 to 1e-8.

I contacted @ovillellas about this, since any change in numbers is concerning. And here is what he told me:

In gvec_to_xy some operations were moved. This includes stuff like changing
the frame of reference used when computing the intersections. Out of
my head, I think I moved the ray-plane intersection to happen in the
detector frame (or maybe the sample frame) instead of happening in lab
frame. That means things are computed using a closer frame, so that
should mean better precision. In any case, it will be worth for a field expert to check it.

So he claims that the difference in numbers is likely because the precision is better now than it was before. But this needs to be verified. The speed improvement is great, and the difference in numbers is small, but we should still verify them.

He also mentioned that gvec_to_xy is the function with the most extensive changes. Indeed, for the other functions, I did not see any difference in the numbers (even down to floating point precision differences). So gvec_to_xy is the main function of difference (and it alone accounted for about half of the 23% speed improvement seen in fit-grains).

@psavery
Copy link
Collaborator Author

psavery commented Apr 4, 2023

Attached below are my results from the analysis on the monolithic ruby example.

new_grains.txt
new_grains_profile.txt

old_grains.txt
old_grains_profile.txt

You can see the difference in values in the two grains.out files (had to be renamed to .txt for GitHub), and you can see the new profiles as well.

psavery and others added 11 commits April 13, 2023 12:04
This will help us keep track of what is being used and what has been
migrated over.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
This copies over files exactly as they are from this repository:

https://github.com/ovillellas/xrd-transforms

At commit: b94f8b2d7839d883829d00a2adc5bec9c80e0116
We don't support older versions of python anymore, so we don't need
these.

This also removes any trailing white space.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Now we compile both the old CAPI extension module and the new one.

Compiling them do not take much time, and this will help us migrate the
functions over more easily.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
I unfortunately did not see a hugely noticeable speedup when I tried this.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Migrate these to their new API functions. Still need some testing
to verify.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
If there's only one task or one worker in the thread pool, just execute
it serially. This removes the overhead of making the thread pool, and
it is better for profiling when running serially.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
make_sample_rmat is the new function that is used for both
make_oscill_rot_mat and make_oscill_rot_mat_array.

I ran fit-grains with these changes and got the same answer.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Newer gcc versions compile code that runs faster. And we want to allow c99 for loops
by default (this will fix a current error).

gcc 10 is what is used by manylinux2014, so it has a good basis for being used.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
@psavery
Copy link
Collaborator Author

psavery commented Apr 18, 2023

@donald-e-boyce tried this out on various examples and approved of the changes.

@psavery psavery merged commit ba59f22 into master Apr 18, 2023
@psavery psavery deleted the new-xrd-transforms branch April 18, 2023 14:35
psavery added a commit that referenced this pull request Apr 27, 2023
This issue arose from #509, due to somewhat complex details.

In short, the old `gvecToDetectorXY()` would filter out g-vectors going
the wrong way (they would result in `nan`), but `gvecToDetectorXYArray()` would
not.

The new `gvec_to_xy()` has behavior matching `gvecToDetectorXYArray()`. But
that means that places where `gvecToDetectorXY()` used to be used, we need
to verify that the function does not receive g-vectors going the wrong way.

Looking through the code, however, I only found one place where
`gvecToDetectorXY()` used to receive g-vectors going the wrong way. It was
the Laue overlay simulator, which would compute g-vectors from symmetrical
HKLs (which is why it would have duplicate g-vectors going the wrong way).

All other cases where `gvecToDetectorXY()` was called would be after
the g-vectors were computed from `anglesToGvec()`, which I believe should
always produce g-vectors going in the correct direction.

As such, we only need to filter out wrong-direction g-vectors in the
Laue simulator.

Fixes: HEXRD/hexrdgui#1412

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
psavery added a commit that referenced this pull request Apr 27, 2023
This issue arose from #509, due to somewhat complex details.

In short, the old `gvecToDetectorXY()` would filter out g-vectors going
the wrong way (they would result in `nan`), but `gvecToDetectorXYArray()` would
not.

The new `gvec_to_xy()` has behavior matching `gvecToDetectorXYArray()`. But
that means that places where `gvecToDetectorXY()` used to be used, we need
to verify that the function does not receive g-vectors going the wrong way.

Looking through the code, however, I only found one place where
`gvecToDetectorXY()` used to receive g-vectors going the wrong way. It was
the Laue overlay simulator, which would compute g-vectors from symmetrical
HKLs (which is why it would have duplicate g-vectors going the wrong way).

All other cases where `gvecToDetectorXY()` was called would be after
the g-vectors were computed from `anglesToGvec()`, which I believe should
always produce g-vectors going in the correct direction.

As such, we only need to filter out wrong-direction g-vectors in the
Laue simulator.

Fixes: HEXRD/hexrdgui#1412

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
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