FFT + Time Reversal Reconstruction #475
Conversation
- added FFT reconstruction - added reconstruction examples
|
Hey @faberno, just pining you on this. Is this something you would like to continue to work on? Best, |
|
Hey @waltsims, sorry for the inactivity, next week I will have time to continue it :) Best, |
|
No rush, just checking in. |
- added notebooks + documentation
|
Hey @waltsims, |
There was a problem hiding this comment.
I added this PML cropping section to the run_simulation function, to have access to the simulation_options. Would you prefer it in the parse_executable_output? (At least, thats where you added an outcommented version of this.)
There was a problem hiding this comment.
Also right now this change breaks the executor test, I will update it when the code is final.
There was a problem hiding this comment.
it would be great if you could make this a private subfunction _remove_pml(...) and place it in parse_executable output for now. That will make future refactoring easier.
There was a problem hiding this comment.
Good idea! I just have the problem, that if _remove_pml(...) is inside parse_executable_output I can't test anymore if the pml is removed on the mock_dict (see: test_sensor_data_cropping_with_pml_outside), because the whole parse_executable_output is patched.
If I create a second private subfunction in parse_executable_output called _load_sensor_data, is there a way to only patch this instead? I didn't find a way to do this.
Or do you see another workaround? :D
|
Hey @faberno Thanks for this work! Before I review your changes, can you make sure your PR passed the CI? It looks like there are some failing tests. Also please be sure to install precommit to ensure your PR is properly formatted. |
- adapt executor tests - removed unused voxel_plot in 3D_FFT example
|
Hey @faberno, Thanks again for the PR. I'll have a look at this this weekend. |
There was a problem hiding this comment.
it would be great if you could make this a private subfunction _remove_pml(...) and place it in parse_executable output for now. That will make future refactoring easier.
|
Please resolve the conflict in the tests and we can merge this in unless you would like to make further changes. |
|
Are there any more tests you would like to have? Otherwise it's fine for me |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #475 +/- ##
==========================================
+ Coverage 72.83% 73.11% +0.28%
==========================================
Files 46 46
Lines 6803 6830 +27
Branches 1304 1308 +4
==========================================
+ Hits 4955 4994 +39
+ Misses 1295 1286 -9
+ Partials 553 550 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hey @waltsims, I added a last test for the coverage. I also made the _remove_pml(...) function a normal member function of Executor (instead of a subfunction in parse_executable output), to still be able to test the cropping. If thats alright for you, everything should be ready :) |
|
This looks great! For now this is on hold until the license is updated. |
|
@faberno can you please resolve the conflicts before I merge this in under the new LGPL-3.0 license? |
# Conflicts: # kwave/executor.py
|
@waltsims Done! |
This PR implements the FFT reconstructions (kspaceLineRecon, kspacePlaneRecon) and fixes Time Reversal.
Added examples:
This PR is just a draft and not ready for review yet!
TODO: