Skip to content

Implement FkRotation as subclass of Rotation by rewriting using a rotation tensor#1772

Merged
APJansen merged 2 commits into
masterfrom
rotation_matrix
Jul 10, 2023
Merged

Implement FkRotation as subclass of Rotation by rewriting using a rotation tensor#1772
APJansen merged 2 commits into
masterfrom
rotation_matrix

Conversation

@APJansen
Copy link
Copy Markdown
Collaborator

Implements an old TODO by @scarlehoff to write the FkRotation layer as a subclass of Rotation.

Unlike for FlavourToEvolution the rotation matrix is fixed, so I thought it made sense to define it right in the class.

Unrelated to this, the docstring isn't accurate, referring to 8 dimensions while actually there are 9. This may be a good occasion to fix it, but I'm not sure what to write as I don't know where the +1 is from, any suggestion?

I've verified that this gives the same output using the script below, and also checked that the new way is about 3-5 times faster.

rot = FkRotation()
test_x = tf.random.normal(shape=(1, 20, 9))

out1 = rot(text_x)
out2 = rot.call_old(test_x)

print(out1 - out2)

@APJansen APJansen added easy Refactoring n3fit Issues and PRs related to n3fit labels Jul 10, 2023
@APJansen APJansen requested a review from scarlehoff July 10, 2023 11:21
@scarlehoff
Copy link
Copy Markdown
Member

Unlike for FlavourToEvolution the rotation matrix is fixed, so I thought it made sense to define it right in the class.

Yes, indeed. And in any case, it is better than the combination "by hand" that we had right now imho. Thanks!

Unrelated to this, the docstring isn't accurate, referring to 8 dimensions while actually there are 9. This may be a good occasion to fix it, but I'm not sure what to write as I don't know where the +1 is from, any suggestion?

The +1 is coming form the addition of V15 to the fit, in order to allow for the charm-anticharm asymmetry a few months ago. During that change it was anticipated that some "8s" would need to be changed by hand, I'm happy that it's only docs for now :P #1629 (comment) so please update the docstr!

@APJansen APJansen merged commit 915d2d6 into master Jul 10, 2023
@APJansen APJansen deleted the rotation_matrix branch July 10, 2023 13:36
@scarlehoff scarlehoff mentioned this pull request Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

n3fit Issues and PRs related to n3fit Refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants