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

Inverse distortion #584

Merged
merged 20 commits into from
Nov 24, 2023
Merged

Inverse distortion #584

merged 20 commits into from
Nov 24, 2023

Conversation

ZackAttack614
Copy link
Collaborator

This PR focuses on the GE inverse distortion function. It has been rewritten into C++ and uses Eigen to do the heavy lifting of the matrix math.

In the multi-ruby example analysis running on my laptop, this PR takes the total runtime from 82 seconds to 62 seconds, cutting out roughly 25% of the runtime.

@HEXRD HEXRD deleted a comment from pep8speaks Nov 17, 2023
Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
@psavery psavery force-pushed the inverse-distortion branch 3 times, most recently from 5a1a659 to 26c10bb Compare November 21, 2023 13:52
Also update the conda build compiler to be C++

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
@HEXRD HEXRD deleted a comment from pep8speaks Nov 21, 2023
This also adds a quick test to verify that it works properly.

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

pep8speaks commented Nov 23, 2023

Hello @ZackAttack614! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-24 20:52:14 UTC

I ran `black -l 80 -S ./test_inverse_distortion.py`

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

psavery commented Nov 23, 2023

I should also test a couple of state files with these changes to verify that there are no issues. I believe this PR means that the transforms functions will now be compiled with a C++ compiler instead of a C compiler. In theory, this shouldn't cause any issues, but I will do a quick check to verify.

After the comments have been addressed and a few state files tested, it will be good to merge!

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
NumpyToNativeEncoder is a custom JSON encoder that encodes numpy types
into native types so it can be saved in JSON.

There is no corresponding decoder, since the JSON just contains native
types and does not need special decoding.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
We can consider other encodings as well. Just need to get the bytes to
a string and back. The raw_unicode_escale appears to work.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
with open(
os.path.join(test_dir, "data", "inverse_distortion_in_out.pkl"), 'rb'
os.path.join(test_dir, 'data', 'inverse_distortion_in_out.json'),
encoding='utf-8'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I shouldn't have to specify the encoding - it uses utf-8 by default.

Copy link
Collaborator

@psavery psavery left a comment

Choose a reason for hiding this comment

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

I tried this out with the single_GE example, and the total time went from 2 minutes and 36 seconds to 2 minutes with the branch!

@psavery
Copy link
Collaborator

psavery commented Nov 24, 2023

I also tested a couple of state files and did not encounter any issues due to the fact that the compiler is C++ instead of C.

@psavery
Copy link
Collaborator

psavery commented Nov 24, 2023

At some point, we can verify that this produces a speed improvement for a real example with many grains, but it looks good, and we can verify that later.

@ZackAttack614 ZackAttack614 merged commit 5f742ea into master Nov 24, 2023
6 checks passed
@ZackAttack614 ZackAttack614 deleted the inverse-distortion branch November 24, 2023 21:49
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.

3 participants