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

waveorder handles all rotational quantities (including retardance) in radians #149

Merged
merged 7 commits into from
Aug 17, 2023

Conversation

talonchandler
Copy link
Collaborator

This PR makes waveorder.models handle all rotational quantities in radians, and adds documentation to make that clear to users.

The only "critical" change is to inplane_oriented_thick_pol3d.apply_inverse_transfer_function:

  • Before this PR: returns retardance in units of wavelength_illumination
  • After this PR: returns retardance in radians.

I'm making this change here for two reasons:

  • so that waveorder is consistent across reconstruction types. For example, the phase reconstructions all return phase values in radians, so it's reasonable to expect all other rotational quantities to be in radians.
  • so that I can handle recOrder #390 (retardance is in um not nm) in the simplest possible way.

To be clear, I think recOrder should continue to return retardance reconstructions in nm (for internal and external consistency), but I also want waveorder to be self consistent.

Comment on lines -102 to -109
retardance = adr_parameters[0] * wavelength_illumination / (2 * np.pi)

# Apply orientation transformations
orientation = stokes.apply_orientation_offset(
adr_parameters[1], rotate=rotate_orientation, flip=flip_orientation
)

return retardance, orientation, adr_parameters[2], adr_parameters[3]
Copy link
Collaborator Author

@talonchandler talonchandler Aug 17, 2023

Choose a reason for hiding this comment

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

Reviewers: this is the "critical" change.

I'm now skipping the multiplication by wavelength_illumination / (2 * np.pi)...

# Apply orientation transformations
orientation = stokes.apply_orientation_offset(
adr_parameters[1], rotate=rotate_orientation, flip=flip_orientation
)

return retardance, orientation, adr_parameters[2], adr_parameters[3]
return adr_parameters[0], orientation, adr_parameters[2], adr_parameters[3]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...and returning the retardance directly.

@talonchandler talonchandler marked this pull request as ready for review August 17, 2023 18:52
@mattersoflight
Copy link
Member

To be clear, I think recOrder should continue to return retardance reconstructions in nm (for internal and external consistency), but I also want waveorder to be self consistent.

What about returning the retardance AND phase in nm from waveOrder. We need wavelength as an input to compute the OTFs, so we have the information at the time of reconstruction to go from radians to nm.

@talonchandler
Copy link
Collaborator Author

What about returning the retardance AND phase in nm from waveOrder.

This is definitely a good alternative, and I don't have a strong preference between returning radians or nm. A few points come to mind:

  • If we decide to perform all reconstructions in nm, we'll also need to constrain all inputs to be in nm (or fixed known units) as well. In some ways this is good (nm in nm out, no fuss), but it's also slightly more restrictive than the current implementation that accepts any units as long as they're consistent. e.g. as long as you enter all of your length inputs (wavelength, pixel size, etc) in a consistent unit (um, nm, km), you'll get the correct unitless answer.

  • If we change to all reconstructions in nm, it will affect our in-progress phase pipelines. @ziw-liu already has a good handle on normalization, so I don't expect this to be hugely disruptive...but it's worth watching out.

  • I have a slight aesthetic preference for an arrays-in-arrays-out library to be flexible on input units (as long as they're consistent) and return unitless things (what I'm proposing in this PR), but I'm quite flexible.

ieivanov
ieivanov previously approved these changes Aug 17, 2023
Copy link
Contributor

@ieivanov ieivanov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mattersoflight mattersoflight left a comment

Choose a reason for hiding this comment

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

good points @talonchandler. The discussion also made me think that we need retardance in radians when we want to compute circular statistics over space or time.

@mattersoflight mattersoflight self-requested a review August 17, 2023 21:16
Copy link
Member

@mattersoflight mattersoflight left a comment

Choose a reason for hiding this comment

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

The docstrings are useful. Please add comments on abbreviations used in method names where needed.

@talonchandler talonchandler merged commit 366ef1b into main Aug 17, 2023
@talonchandler talonchandler deleted the retardance-in-radians branch August 17, 2023 22:37
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