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

KWave adapter rotation bug fix #270

Merged
merged 4 commits into from
Jun 7, 2024
Merged

KWave adapter rotation bug fix #270

merged 4 commits into from
Jun 7, 2024

Conversation

TomTomRixRix
Copy link
Collaborator

@TomTomRixRix TomTomRixRix commented Mar 15, 2024

I switched rot90 to transpose transformation and deleted the reverse ordering of sensor elements in time series data.

So far not test have been run. It might make sense to implement additional tests for avoiding such bugs.

Please check the following before creating the pull request (PR):

  • Did you run automatic tests?
  • Did you run manual tests?
  • Is the code provided in the PR still backwards compatible to previous SIMPA versions?

List any specific code review questions

List any special testing requirements
Please also test 3D case, this hasn't been done so far.

Additional context (e.g. papers, documentation, blog posts, ...)

Provide issue / feature request fixed by this PR

Fixes #264

@TomTomRixRix TomTomRixRix added the bug Something isn't working label Mar 15, 2024
@TomTomRixRix TomTomRixRix self-assigned this Mar 15, 2024
@TomTomRixRix TomTomRixRix requested a review from kdreher March 15, 2024 17:09
@TomTomRixRix TomTomRixRix changed the base branch from main to develop March 15, 2024 17:12
@TomTomRixRix
Copy link
Collaborator Author

TomTomRixRix commented Mar 22, 2024

Currently manual test simpa_tests/manual_tests/acoustic_forward_models/KWaveAcousticForwardConvenienceFunction.py fails, see #271. But this is likely not related to the changes from this PR, it already fails on develop.

@TomTomRixRix
Copy link
Collaborator Author

TomTomRixRix commented Mar 22, 2024

simpa_tests/manual_tests/digital_device_twins/SimulationWithMSOTInvision.py also doesn't work but probably not related to this PR

@TomTomRixRix TomTomRixRix marked this pull request as ready for review March 22, 2024 17:21
@faberno
Copy link
Collaborator

faberno commented Mar 25, 2024

There are still some rot90 left in simpa, mostly in the plotting functions and some tests (also in KWaveAcousticForwardConvenienceFunction, which is one reason why the output looks so strange). We should probably replace them too :D

@TomTomRixRix
Copy link
Collaborator Author

There are still some rot90 left in simpa, mostly in the plotting functions and some tests (also in KWaveAcousticForwardConvenienceFunction, which is one reason why the output looks so strange). We should probably replace them too :D

You were right, the rot90 error was also a problem in the KWaveAcousticForwardConvenienceFunction test. Furthermore the field of view and the spacing needed to be set in order to have a visually similar reconstruction. Now #271 should also be fixed.

@jgroehl jgroehl merged commit c86020b into develop Jun 7, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible Bug: Rot90 in kwave adapter
3 participants