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

BREAK: remove spin projections in cons rules #282

Merged
merged 18 commits into from
Aug 26, 2024

Conversation

grayson-helmholz
Copy link
Contributor

@grayson-helmholz grayson-helmholz commented Aug 26, 2024

Closes #280

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@grayson-helmholz grayson-helmholz added ⚠️ Interface Breaking changes to the API ⚙️ Enhancement Improvements and optimizations of existing features labels Aug 26, 2024
@grayson-helmholz grayson-helmholz changed the title Remove spin projections in cons rules BREAK: remove spin projections in cons rules Aug 26, 2024
ingoing_spins: List[SpinEdgeInput],
outgoing_spins: List[SpinEdgeInput],
ingoing_spin_magnitudes: List[EdgeQN.spin_magnitude],
outgoing_spin_magnitudes: List[EdgeQN.spin_magnitude],
interaction_qns: SpinMagnitudeNodeInput,
Copy link
Member

Choose a reason for hiding this comment

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

Something I realize now is that this rule seems to be checking two things and therefore needs $L$ and $S$ of the nodes. It would be better to split that.

Maybe we should write an issue for this if this rule can be split up.

Copy link
Member

Choose a reason for hiding this comment

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

This is also not really a suitable class name:

class SpinMagnitudeNodeInput:
l_magnitude: NodeQN.l_magnitude = field(converter=NodeQN.l_magnitude)
s_magnitude: NodeQN.s_magnitude = field(converter=NodeQN.s_magnitude)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah seems fishy to me. @grayson-helmholz could you post an issue to split this rule into spin magnitude and $LS$ magnitude.

angular_momentum=Spin(ang_mom_mag, 0),
coupled_spin=Spin(coupled_spin_mag, -1),
),
([1], [spin2_mag, 1], SpinNodeInput(ang_mom_mag, 0, coupled_spin_mag, -1)),
Copy link
Member

Choose a reason for hiding this comment

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

Kind of hard to read this test, but it was already like that unfortunately

@redeboer
Copy link
Member

This branch is currently being tested with AmpForm here:
https://github.com/ComPWA/ampform/actions/runs/10562139178

Copy link
Member

@redeboer redeboer left a comment

Choose a reason for hiding this comment

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

LGTM 🍰

@grayson-helmholz grayson-helmholz merged commit 924548f into main Aug 26, 2024
21 checks passed
@grayson-helmholz grayson-helmholz deleted the remove_spin-projections_in_cons-rules branch August 26, 2024 15:06
@redeboer redeboer added this to the 0.11.0 milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ Enhancement Improvements and optimizations of existing features ⚠️ Interface Breaking changes to the API
Projects
None yet
2 participants