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

Segment by Segment - the backend #474

Open
wants to merge 80 commits into
base: master
Choose a base branch
from
Open

Conversation

JoschD
Copy link
Member

@JoschD JoschD commented Nov 22, 2024

This is the Segment-by-Segment algorithm as envisioned by @jaimecp89.
In this state it should contain the same functionality as the SbsPhaseAdvance (#308) branch, but lacks a test, some doc-strings and some cleanup.

Advantages:

  • Separated model creation, segment input and segment-by-segment propagation
  • Object Oriented:
    • One only needs to add a Propagable object to add a parameter, which can then be just added to the loop.
    • Highly shared functionality between parameters
  • Uses the Optics-Measurements TfsCollection
  • Allows for Element-Segments as input (see next point)
  • Finds BPMs around from model

Caveats: (i.e. breaking backwards compatibility with BB.src SbS)

  • Standardized and simplified output names
  • Standardized ini_r## and end_r## to r##_ini and r##_end (as all the other parameters)
  • Renamed front (or omission of it) to forward and back to backward everywhere
  • Some changes in the Model Creation
    • some paths (model-dir)
    • call-logic: see full_run function and post_run function for checks

Todo:

  • Move segment_by_segment.py to main folder.
  • move the segment_by_segment/matcher and sbs_matcher.py to new branch (accounts for a lot of changed files, make new issue to adapt it to python3)
  • remove debug logging
  • Add Test (see text from Tobias below)
  • Add doc-strings and type-hints
  • Test with segment-by-segment GUI from omc3-gui package

From Tobias :

I put an example to start testing the sbs on my work public /afs/cern.ch/work/t/tpersson/public/forSBStest_ACdipole/

There are some files also to generate other tracking but the important just to test the SBS are the files in the measured optics such as total_phase_x.tfs etc. You can compare the result to the output in sbs folder there. The starting point used were:

BPM.12L1.B1
BPM.12R1.B1

And the correction used for the sbs is in the file: sbs/corrections_IP1.madx

The modifier used was the opticsfile.22.

This is a good first test but then the error propagation also needs to be checked. For my version I checked this by analyzing files from Beta-Beat.src and then compared it to the OMC3 with the option beta-beat.src “names”.

tpersson and others added 30 commits July 6, 2021 11:08
# Conflicts:
#	omc3/model/accelerators/lhc.py
#	omc3/model/model_creators/lhc_model_creator.py
#	omc3/model/model_creators/ps_model_creator.py
#	omc3/model/model_creators/psbooster_model_creator.py
#	omc3/model/model_creators/segment_creator.py
#	omc3/model_creator.py
# Conflicts:
#	omc3/model/accelerators/lhc.py
#	omc3/model/model_creators/lhc_model_creator.py
#	omc3/optics_measurements/constants.py
@JoschD JoschD requested a review from a team as a code owner March 3, 2025 13:48
@JoschD
Copy link
Member Author

JoschD commented Mar 3, 2025

Implemented

  • Updated MAD-X binaries to 5.09 -> needed to adapt some checks in the correction tests
  • Made the entry points lists when calling e.g. python -m omc3, python -m omc3.scripts and python -m omc3.plotting
  • Separated MAD-X code and accelerator class: The MAD-X code is now fully moved to the respective model creators, as we need to make one per accelerator anyway.
    The accelerator class should now only contain attributes that describe the accelerator.
    THIS IS A BIG CHUNK OF THE CHANGES!
  • Model creation is also a bit updated and has a full_run() function on the creators, that do the individual steps themselves (before that they were called from the outside). It is not perfect, as there is also the prepare_options function, which for the creators that can use acc-models need to be called beforehand with the fetcher options, that are only available "on the outside".
    It is still a bit weird with the list_choices but hopefully much better and more followable than before.
  • Implemented the Segment-by-Segment in two parts:
    • The SegmentCreators: Which are the model creators which output twiss files of the Segements, in which the measurements are propagated
    • The SbS-Analysis, which is taken from Jaimes implementation that never quite made it to BBS: This defines propagables from the measurement data, whose functions are twofold: a) they provide the Segment Creators with the initial conditions to be propagated. and b) They take the propagated segment twiss output, compare it to the model and write out the differences to files (which can then be loaded in the omc3_gui).

I also decided not to implement a test that compares the SbS output from BBS with the one from omc3.
Tobias had started with one, but only for beam1 and for measurement data created from tracking.
There was a warning in the madx script that it will not work with beam2, so I didn't know how I would even create this.
Also I couldn't get the BBS-SbS to run with the track-one output.
In the end, I checked the BBS-SbS output Tobias had already created against the output form my omc3 implementation and it was identical. Then I ran BBS-SbS on some old measurement data and compared it also against omc3 and again it was identical. This needs to be repeated for coupling propagation as well.

The test I now implemented takes a similar approach to Tobias' original test (i.e. also how we create global correction tests), only that I create the twiss-with-errors via our omc3 model creator, and then use the fake-measurement-from-model script to create the measurement data that is then used as SbS input. This at least verifies that a) the results look reasonable and b) the propagation is identical to the fake model when the same errors are used as corrections.

Missing:

The following features are still missing, but as this is already a working sbs I would merge this first and add the other features later. In particular the coupling needs to be done before commissioning, but I will implement some SPS stuff first:

  • Coupling init-parameters and propagation
  • Segment-by-Segment matcher for automatic calculation of corrections

@fsoubelet
Copy link
Member

Closes #352

@fsoubelet fsoubelet linked an issue Mar 4, 2025 that may be closed by this pull request
Copy link
Contributor

@jgray-19 jgray-19 left a comment

Choose a reason for hiding this comment

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

Mostly extremely pedantic suggestions. Not sure I can comment on much on the actual code, but I think most of it looks like some nice improvements. Hopefully this will also make moving to xsuite easier. So, please excuse my pedantry and lacking in understanding.

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.

Nice. Big PR means big review, so there are plenty of comments but nothing major in the design / execution :)

Sbs_propagation script is a rough read with all the jumping around to function definitions due to the level of desired flexibility here (finding closest, segment extension, segment definitions and parsing etc). Maybe one day we improve on that.

Approval requires a successful demo with the GUI on top of this ;)

Reminders - to bring up in tomorrow's meeting:

  • Should response_madx run with best-knowledge model? (currently commented out, uses nominal model)
  • Should we normalize models on using absolute paths everywhere? (imo this is less readable in the scripts, much better for reproducibility)
  • Lukas's concerns on the isolation forest (is it ticked ON by default in the GUI?)

@JoschD
Copy link
Member Author

JoschD commented Mar 5, 2025

Closes #331

@JoschD
Copy link
Member Author

JoschD commented Mar 5, 2025

Closes #109

@JoschD
Copy link
Member Author

JoschD commented Mar 5, 2025

Closes #469

@JoschD JoschD requested review from fsoubelet and jgray-19 March 5, 2025 18:28
jgray-19
jgray-19 previously approved these changes Mar 6, 2025
Copy link
Contributor

@jgray-19 jgray-19 left a comment

Choose a reason for hiding this comment

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

If the tests pass 👍

Copy link

codeclimate bot commented Mar 6, 2025

Code Climate has analyzed commit 2e82892 and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 8

The test coverage on the diff in this pull request is 82.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 86.0% (-0.2% change).

View more on Code Climate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Physics The implementation is closely related to new physics to be used. Estimate: Complicated Might need some major overhaul of the code. Priority: Medium Work on this. Type: Feature A (suggetion for a) new feature or enhancement in functionality.
Projects
None yet
4 participants