Skip to content

Conversation

@jgroehl
Copy link
Collaborator

@jgroehl jgroehl commented Nov 18, 2025

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

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

Provide issue / feature request fixed by this PR

Fixes #412

… re-happening, and adjusted the tissue value tests such that the values are in line with the paper of BASHKATOV et al., 2011

Fixes #412
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a critical bug in the Rayleigh scattering equation (issue #412) where an exponent was incorrectly specified as 1e-4 instead of -4. The fix is accompanied by comprehensive test coverage and related improvements to device parameters, documentation, and code quality.

  • Main fix: Corrected Rayleigh scattering exponent from 1e-4 to -4 in scattering_from_rayleigh_and_mie_theory
  • Added comprehensive test suite to prevent regression of the scattering equation bug
  • Updated literature values and device parameters to match corrected scattering model

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
simpa/utils/libraries/spectrum_library.py Critical bug fix: Corrected Rayleigh scattering exponent from 1e-4 to -4
simpa_tests/automatic_tests/tissue_library/test_scattering_equation.py Added comprehensive test coverage for scattering equation to prevent regression
simpa/utils/calculate.py Fixed rotation_matrix_between_vectors to properly handle parallel and anti-parallel vectors
simpa/utils/libraries/literature_values.py Updated BMIE coefficients for epidermis and dermis to align with corrected scattering model
simpa_tests/test_utils/tissue_composition_tests.py Adjusted reference scattering value and improved test infrastructure with matplotlib backend
simpa/core/simulation_modules/optical_module/optical_adapter_base.py Enhanced laser pulse energy handling to support wavelength-dependent energy values
simpa/core/simulation_modules/reconstruction_module/*.py Added indexing='ij' parameter to torch.meshgrid calls for PyTorch 2.0+ compatibility
simpa/utils/settings.py Added get() method for dictionary-like default value handling
simpa/utils/tags.py Clarified documentation for LASER_PULSE_ENERGY_IN_MILLIJOULE to indicate iterable support
simpa/core/device_digital_twins/pa_devices/ithera_msot_acuity.py Updated bandwidth (55% → 153%) and corrected laser position calculation for device accuracy
simpa/core/simulation_modules/acoustic_module/simulate_2D.m Fixed comment: "minimum CFL" → "maximum CFL"
simpa/core/processing_components/monospectral/field_of_view_cropping.py Corrected docstring to accurately describe field of view cropping functionality
simpa_tests/automatic_tests/tissue_library/test_tissue_library_against_literature_values.py Removed temporary workaround for Intel architectures

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jgroehl jgroehl changed the base branch from main to develop November 18, 2025 12:52
@jgroehl
Copy link
Collaborator Author

jgroehl commented Nov 18, 2025

All literature values are quite hand-wavy within SIMPA to be fully honest.
I have arrived at a middle ground between the values reported by Jacques and the ones reported by Bashkatov et al.
I believe that the updated values are now more consistent than before and still well within the Variability of values reported in the literature.

@jgroehl jgroehl added the bug Something isn't working label Nov 18, 2025
Copy link
Collaborator

@TomTomRixRix TomTomRixRix left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm not an expert on the tissue parameters, but the implementation makes sense.

get_fully_deoxygenated_blood_reference_dictionary, \
get_lymph_node_reference_dictionary

# FIXME temporary workaround for newest Intel architectures
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was the intel architectures issue solved or do we simply not care anymore?

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.

4 participants