Small bug fixes

[faberno]

Hey, this PR fixes some small bugs that caused problems:

• Alpha Coefficient can be 0 (kspaceFirstOrder_inputChecking.m:216)
• blank_sensor() should return True, if no fields are set and !time_rev (kspaceFirstOrder_inputChecking.m:482)
• for the 2d case expand_matrix uses exp_coeff as pad_width, but this is equivalent to [(exp_coeff[0], exp_coeff[1]), (exp_coeff[0], exp_coeff[1])] while it should be [(exp_coeff[0], exp_coeff[0]), (exp_coeff[1], exp_coeff[1])]

[faberno]

I also noticed, that kspaceFirstOrderxD mutates its inputs, which is not the case in the matlab version. Is this your desired behaviour? If you run e.g. the at_array_as_sensor example with pml_inside=False, the source.p0 gets expanded, which leads to a crash in the second kspaceFirstOrder2D. To fix this we could copy some of the inputs at the beginning of kspaceFirstOrderxD.

[waltsims]

Hi @faberno,

Thanks for the PR! I appreciate you adding these small fixes. Please have a look at the failing tests before I review you PR.

Thanks, Walter

[waltsims]

I also noticed, that kspaceFirstOrderxD mutates its inputs, which is not the case in the matlab version. Is this your desired behaviour? If you run e.g. the at_array_as_sensor example with pml_inside=False, the source.p0 gets expanded, which leads to a crash in the second kspaceFirstOrder2D. To fix this we could copy some of the inputs at the beginning of kspaceFirstOrderxD.

Please open an issue with a minimally reproducible example for this and we can discuss further there.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.09%. Comparing base (8eca448) to head (cd864fc). Report is 38 commits behind head on master.

Files with missing lines	Patch %	Lines
kwave/kWaveSimulation.py	66.66%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
+ Coverage   71.84%   72.09%   +0.24%     
==========================================
  Files          46       46              
  Lines        6744     6747       +3     
  Branches     1496     1496              
==========================================
+ Hits         4845     4864      +19     
+ Misses       1333     1326       -7     
+ Partials      566      557       -9     

Flag	Coverage Δ
3.10	72.29% <83.33%> (+0.25%)	:arrow_up:
3.11	72.29% <83.33%> (+0.25%)	:arrow_up:
3.12	72.29% <83.33%> (+0.25%)	:arrow_up:
3.9	72.06% <83.33%> (+0.24%)	:arrow_up:
macos-latest	72.03% <83.33%> (+0.24%)	:arrow_up:
ubuntu-latest	72.06% <83.33%> (+0.24%)	:arrow_up:
windows-latest	72.07% <83.33%> (+0.24%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[faberno]

Hey @waltsims, I think a new test has be created to cover the blank_sensor line, by e.g. starting a simulation where only p_final is recorded. Where should I add such a test?

[waltsims]

Feel free to make a new file called test_simulation.py in k-wave-python/tests/. Have a look at k-wave-python/tests/test_checks.py and k-wave-python/tests/test_executor.py for inspiration. Let me know if you have any other questions! Thanks!

[waltsims]

Hey @faberno, until now, we have not run simulations in the CI but mocked inputs and outputs. That is why the dependencies for the executables are not found and failing.

You could mock the behavior you are trying to test in units, or, for a small simulation, update the workflow to install the required dependencies and run the simulation as an integration test.

[faberno]

Ahh good to know, I will change it :)

[waltsims]

I also noticed, that kspaceFirstOrderxD mutates its inputs, which is not the case in the matlab version. Is this your desired behaviour? If you run e.g. the at_array_as_sensor example with pml_inside=False, the source.p0 gets expanded, which leads to a crash in the second kspaceFirstOrder2D. To fix this we could copy some of the inputs at the beginning of kspaceFirstOrderxD.

I've opened a separate issue for this as well