-
Notifications
You must be signed in to change notification settings - Fork 25
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
Inverse distortion #584
Conversation
Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
5a1a659
to
26c10bb
Compare
Also update the conda build compiler to be C++ Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
26c10bb
to
daafa05
Compare
Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
This also adds a quick test to verify that it works properly. Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
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>
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
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. |
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. |
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.