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

Added IRNL correction script #74

Merged
merged 19 commits into from Nov 15, 2021
Merged

Added IRNL correction script #74

merged 19 commits into from Nov 15, 2021

Conversation

JoschD
Copy link
Member

@JoschD JoschD commented Jun 14, 2021

Closes #73

@JoschD JoschD self-assigned this Jun 14, 2021
@mihofer
Copy link
Contributor

mihofer commented Jun 15, 2021

maybe adding a simple test(-s) to check and as examples
for some simple cases, one may also check that the corr values matches the integrated err of the triplet magnets

@JoschD JoschD requested a review from mihofer November 10, 2021 15:58
@mihofer
Copy link
Contributor

mihofer commented Nov 11, 2021

What changes would be required to adapt this to a different machine, say FCC-hh, or a different configuration? Could be interesting to highlight this in some comments.

mihofer
mihofer previously approved these changes Nov 11, 2021
Copy link
Contributor

@mihofer mihofer left a comment

Choose a reason for hiding this comment

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

Very superficial reading has shown no major critiqueable points

@JoschD
Copy link
Member Author

JoschD commented Nov 11, 2021

What changes would be required to adapt this to a different machine, say FCC-hh, or a different configuration? Could be interesting to highlight this in some comments.

From the top of my head I can think of only two (and a half) things:

minor:

  • add machine as accel input parameter/change allowed IP numbers
  • define standard RDTs to correct (depend a bit on where the working point of the machine will be)

a bit more cumbersome:

  • so far it only knows the names of the corrector magnets for lhc/hllhc so at that point you would have to have another map from corrector order to corrector name (e.g. b4 -> MCOXF?.3[LR]\d)

If the magnets are similarly named i.e. end in [LR]\d(\.b\d)?(\.\.\d+)?, the algorithm can find the magnets belonging to each side of the IPs and things should work. If the naming scheme is different, this also needs to be adapted.

But all of this is "just" naming conventions. Once it is clear which magnets are the correctors and which magents belong to which side of which IP, the rest should work automatically.

fsoubelet
fsoubelet previously approved these changes Nov 15, 2021
Copy link
Member

@fsoubelet fsoubelet left a comment

Choose a reason for hiding this comment

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

Some minor detail comments but looks great to me! Merge away when you want.

If you have some extra time on your hands it would be nice to write a bit about this in the OMC website, but maybe when the paper is published then it will be enough of a documentation by itself.

Sorry for the late review :)

pylhc/irnl_rdt_correction.py Show resolved Hide resolved
pylhc/irnl_rdt_correction.py Show resolved Hide resolved
pylhc/irnl_rdt_correction.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
def test_rdts(self, tmp_path: Path):
"""Test that different RDTs can be corrected and only their correctors
are returned. Also checks that the corrector values are varying between RDTs
when they should. Octupole RDTs are used for this example.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, if it is not too much work, in time we parametrize this test to also include different orders. Definitely not merge blocking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but I don't think we need to test EVERY case that makes sense physically. It's more important to test different ways the algorithm works (and already here I have quite a few redundancies in the tests)

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two things I should test for though:

  • use feed-down from corrector to correct lower order rdts
  • use two sets of optics/RDTs to correct for at the same time.

Maybe I will add these before merge

@JoschD JoschD dismissed stale reviews from fsoubelet and mihofer via e2fe42a November 15, 2021 12:30
mihofer
mihofer previously approved these changes Nov 15, 2021
@JoschD JoschD merged commit b62b88e into master Nov 15, 2021
@JoschD JoschD deleted the feature/73/irnl_correction branch November 15, 2021 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IRNL correction script
3 participants