Skip to content

Conversation

@nkhusid
Copy link
Contributor

@nkhusid nkhusid commented Jun 9, 2025

For the aligned polarization model, the A parameter is different from that of the generic model; when enforcing equatorial plane symmetry (the aligned model), the angular factors $\enspace_{-2}Y_{\ell m} \enspace$ are factored out of the mode amplitudes, whereas no constraints on polarizations (the generic model) allows us to absorb the angular factors into the intrinsic mode amplitudes. I have written a method in result.py called get_generic_amplitude() which converts the A outputted by the aligned model into that of the generic model.

@maxisi
Copy link
Owner

maxisi commented Jun 9, 2025

great! a few requests:

  1. shouldn't this just return a if the generic model was used rather than failing?
  2. the mode object has a get_coordinate() method which is what you should use to get the byte string that indexes the mode in the arViz file (you should not build the bytes manually, because we might change how the indexing is done and then your code would break --- but get_coordinate will always work)

@maxisi maxisi requested a review from Copilot June 9, 2025 12:30
@maxisi maxisi self-assigned this Jun 9, 2025
@maxisi maxisi added the enhancement New feature or request label Jun 9, 2025
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 introduces a new method, get_generic_amplitude(), in result.py to convert amplitudes from the aligned model to those of the generic model by integrating the angular factors.

  • Added get_generic_amplitude() method
  • Converts aligned model "A" values to generic model amplitudes by computing angular factors
  • Validates input by checking for the existence of the "cosi" key in the posterior data
Comments suppressed due to low confidence (1)

ringdown/result.py:976

  • Consider adding a docstring for get_generic_amplitude() to describe its purpose, input expectations, and returned output for future maintainability.
def get_generic_amplitude(self):

cosi = self.posterior.cosi.values
swsh = utils.swsh.construct_sYlm(-2, mode.l, mode.m)
ylm_p = swsh(cosi)
ylm_m = swsh(-cosi)
Copy link

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

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

Ensure that the shapes of ylm_p, ylm_m, and C are consistent for element-wise multiplication to avoid broadcasting issues; consider asserting or documenting the expected dimensions.

Suggested change
ylm_m = swsh(-cosi)
ylm_m = swsh(-cosi)
# Assert shape consistency
assert C.shape == ylm_p.shape == ylm_m.shape, (
f"Shape mismatch: C.shape={C.shape}, ylm_p.shape={ylm_p.shape}, ylm_m.shape={ylm_m.shape}"
)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants