Skip to content

feat: port lps2sph/sph2lps coordinate transforms from MATLAB#476

Open
AmitSubhash wants to merge 2 commits into
OpenwaterHealth:mainfrom
AmitSubhash:add-lps-spherical-transforms
Open

feat: port lps2sph/sph2lps coordinate transforms from MATLAB#476
AmitSubhash wants to merge 2 commits into
OpenwaterHealth:mainfrom
AmitSubhash:add-lps-spherical-transforms

Conversation

@AmitSubhash

Copy link
Copy Markdown

Closes #463

Ports fus.seg.lps2sph and sph2lps from openwater-neuromod-software into
openlifu.geo.transforms, right next to the existing cartesian/spherical
converters. Scalar versions plus vectorized ones for arrays.

The main thing here was matching the MATLAB convention exactly: angles in
degrees, the +90 azimuth offset (0 is the anterior/nose line, increasing
toward patient-left), and phi as an elevation angle rather than the polar
angle off z. Return order follows the MATLAB ([th, phi, r] and [l, p, s]).

These are deterministic transforms with no captured fixtures, so I tested
them analytically like you suggested in the issue:

  • round-trip identity across all 8 octants, both directions
  • known values hand-derived from the MATLAB (pure-left, pure-superior, a
    45 degree case, a 3-4-5 case)
  • edge cases: r=0, on-axis points, negative coords

I also checked the outputs against Octave's cart2sph/sph2cart to be sure the
convention lines up. pytest passes locally.

Happy to move the functions if you'd rather they live somewhere else.

Port fus.seg.lps2sph and fus.seg.sph2lps from the legacy MATLAB toolbox
(opw_neuromod_sw) into openlifu.geo.transforms, with scalar and vectorized
variants matching the existing cartesian/spherical converters. Faithfully
preserves the MATLAB convention (degrees, azimuth+90 offset, elevation angle).
Adds golden-value, round-trip, edge-case (r=0), and vectorized-consistency
tests to test_geo.py.

Refs OpenwaterHealth#463
@AmitSubhash

Copy link
Copy Markdown
Author

Hi @ebrahimebrahim, gentle nudge on this one when you get a chance. It ports the lps2sph / sph2lps coordinate transforms from MATLAB into openlifu.geo, with tests in tests/test_geo.py. I've just merged main in so the branch is up to date.

Since this is my first PR here, CI looks like it's waiting on approval to run, so it hasn't reported any checks yet. Could you kick off the workflow and take a look when you have a moment? Happy to adjust anything. Thanks!

@ebrahimebrahim

Copy link
Copy Markdown
Collaborator

Is there a planned use for these functions?

@peterhollender

Copy link
Copy Markdown
Contributor

Is there a planned use for these functions?

I know they used to be a part of the virtual fitting, which would go between volume-space coordinates on the surface of the head and the polar coordinate search grid. I think you guys ported the virtual fitting code- were those converters not needed with your approach?

@ebrahimebrahim

Copy link
Copy Markdown
Collaborator

We added spherical-cartesian converters in https://github.com/OpenwaterHealth/openlifu-python/blob/main/src/openlifu/geo/transforms.py without mention of RAS/LPS. The RAS/LPS aspect is a particular way to interpret the space anatomically, which can come into play in the virtual fitting code because it is provided an affine

@AmitSubhash

Copy link
Copy Markdown
Author

Thanks @ebrahimebrahim, @peterhollender, fair question.

Context: I picked this up as a good first issue (#463), scoped and assigned by @dblizinski in the MATLAB to Python migration lane, to port +fus/+seg/lps2sph.m / sph2lps.m preserving the MATLAB convention (degrees, +90 deg azimuth from the nose line, elevation from the eye line). #476 does that, with analytic tests (round-trips across octants, hand-derived known values, r=0 / on-axis / negatives); CI is green now.

On the actual question, I traced how coordinates flow on main: geo/transforms.py already has generic radian spherical<->cartesian converters, run_virtual_fit works in RAS via volume_affine_RAS, and LPS only appears at the DICOM boundary, resolved by an affine flip (np.diag([-1, -1, 1, 1]) in volume_conversion.py), not a spherical conversion. So there is no current consumer for an LPS-specific spherical convention.

So the call is yours, since you own the virtual-fit port: resolve orientation purely through the affine, or keep a small, clearly labeled LPS-parity helper for cross-validating the port against the legacy tool's angles? Happy to shape #476 either way, a documented parity utility, folded in wherever it fits, or set aside if RAS already covers it.

Either way, glad to have picked up the RAS/LPS context from your discussion.

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.

Port one MATLAB calibration utility to Python (good first issue)

3 participants