Skip to content

Fix orbcorr cavity#286

Merged
Gabrielrezende-asc merged 5 commits intomasterfrom
fix-orbcorr-cavity
Jun 18, 2025
Merged

Fix orbcorr cavity#286
Gabrielrezende-asc merged 5 commits intomasterfrom
fix-orbcorr-cavity

Conversation

@Gabrielrezende-asc
Copy link
Copy Markdown
Contributor

A possible problem in the orbit correction class:

  1. The cavity was always being set as True regardless of the initial state of the model, and the original state was not being restored.
  2. The cavity must be set to True when calculating the kicks to correct the orbit if we choose to use the RF correction, so I added a line to set the cavity to True in this function and then restore the original value.

@xresende
Copy link
Copy Markdown
Contributor

xresende commented Nov 22, 2024

should we keep modifying the model under the hood for the use of algorithms to make sense or should we raise an explanatory exception educating the user on the propor use of the algorithm ?

@murilobalves , @fernandohds564 , @vellosok75 , @Gabrielrezende-asc

another concern: potential inconsistencies because of temporary modifications of the model when threads that use the same model are running.

@fernandohds564
Copy link
Copy Markdown
Contributor

I think we should drop this behavior and raise exceptions when the model state doesn't match what is expected.

@xresende
Copy link
Copy Markdown
Contributor

@Gabrielrezende-asc

I think we should drop this behavior and raise exceptions when the model state doesn't match what is expected.

agreed

@xresende
Copy link
Copy Markdown
Contributor

@murilobalves , @vellosok75 , do you want to take a position on this ?

@Gabrielrezende-asc , if others do not object, I think we can proceed with what fernando and I agreed.

Copy link
Copy Markdown
Contributor

@xresende xresende left a comment

Choose a reason for hiding this comment

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

If no one object we should implement what was discussed in the conversation.

@vellosok75
Copy link
Copy Markdown
Contributor

@murilobalves , @vellosok75 , do you want to take a position on this ?

@Gabrielrezende-asc , if others do not object, I think we can proceed with what fernando and I agreed.

I agree. I prefer raising exceptions.

@@ -262,6 +261,8 @@ def set_delta_kicks(self, dkicks):
def set_kicks(self, kicks):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, shouldn't we be working with the set_delta_kicks method in correct_orbit? I believe the set_kicks is incorrect since the pseudo-inversion is giving us deltas, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This function applies the kicks. The delta kick values are computed and processed before this function is called.

@murilobalves
Copy link
Copy Markdown
Contributor

should we keep modifying the model under the hood for the use of algorithms to make sense or should we raise an explanatory exception educating the user on the propor use of the algorithm ?

@murilobalves , @fernandohds564 , @vellosok75 , @Gabrielrezende-asc

another concern: potential inconsistencies because of temporary modifications of the model when threads that use the same model are running.

I agree with raising exceptions.

@Gabrielrezende-asc
Copy link
Copy Markdown
Contributor Author

Ok guys, as soon as I finish tasks related to the VPU commissioning I will continue this PR

Co-authored-by: fernandohds564 <fernandohds564@gmail.com>
@Gabrielrezende-asc Gabrielrezende-asc merged commit 22dc81d into master Jun 18, 2025
@Gabrielrezende-asc Gabrielrezende-asc deleted the fix-orbcorr-cavity branch June 18, 2025 12:13
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.

5 participants