Skip to content

Conversation

@tomelse
Copy link

@tomelse tomelse commented Jan 20, 2025

Your formula for Rayleigh scattering used to be ~ (lambda) ^ (1e-4), but should be ~ (lambda)^-4

I put the pull request in by just editing the GitHub text file, so haven't rerun the tests, but I can't see why this should break anything. If you have any optical tests that validate light fluence values etc, then these will need remaking I suspect.

@tomelse
Copy link
Author

tomelse commented Jan 20, 2025

P.s. I think I raised this issue a few years ago, but it didn't get fixed, so it might be worth propagating this correction to all other branches to make sure that it doesn't somehow get overwritten in future?

@kdreher
Copy link
Collaborator

kdreher commented Jan 21, 2025

Yes, you're absolutely right. Not only this but also, everywhere we use this function, we use it as scattering coefficient instead of the reduced scattering.

We're directly testing certain scattering coefficients, so the tests shouldn't break. It seems like some builds don't run at the moment, which is a different problem, though 😅

jgroehl
jgroehl previously approved these changes Nov 17, 2025
@jgroehl jgroehl changed the base branch from main to develop November 17, 2025 15:32
@jgroehl jgroehl dismissed their stale review November 17, 2025 15:32

The base branch was changed.

@jgroehl
Copy link
Collaborator

jgroehl commented Nov 18, 2025

@kdreher The tests are indeed failing because of mismatches in the created versus tested scattering.
If you are correct in stating that we use reduced scattering and scattering interchangeably and wrong within the simulations, we should look into this.

@jgroehl
Copy link
Collaborator

jgroehl commented Nov 18, 2025

At the very least, a test should be added that tests for the correct implementation of this, such that it fails in the future when it accidentally gets reverted. Also: the windows tests should not succeed...!

@jgroehl
Copy link
Collaborator

jgroehl commented Nov 18, 2025

The scattering equation itself does not care if we scale the reduced scattering or the actual scattering coefficient:
image
It all depends on if the input a represents reduced or normal scattering.

The SIMPA function does not mention "reduced scattering" in its documentation, but refers to "scattering".
If we use it to determine wavelength-dependent scattering, I think that is completely fine - we only run into problems if we expect the output to be in reduced scattering units even though we define a in scattering or vice versa.

@jgroehl
Copy link
Collaborator

jgroehl commented Nov 18, 2025

I have double-checked, and we are defining a as "scattering at 500nm" and have converted literature values accordingly throughout the literature library and the use is consistent and correct in "normal scattering" throughout SIMPA!

jgroehl added a commit that referenced this pull request Nov 18, 2025
… 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
@jgroehl jgroehl mentioned this pull request Nov 18, 2025
3 tasks
@jgroehl
Copy link
Collaborator

jgroehl commented Nov 18, 2025

Integrated this change with a whole test suite in PR #432

@jgroehl jgroehl closed this Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants