Arc corr and glob check fix #437
Conversation
|
Hey @fscarlier . This still needs tests, have you progressed in that regard? |
|
Hey @fscarlier . It would be great if you could implement some tests, so we can merge this back into master and avoid the clusterfudge we had last year with diverging branches. |
|
Hey @fscarlier what is the status of these tests? Have you talked to @emaclean or @rogeliotomas that maybe someone else can take over this task if this cannot be done by you? |
|
after mergin with master I had to delete the corrections.json files. If there is no stuff, needs to be checked |
| @@ -1,5 +1,19 @@ | |||
| # OMC3 Changelog | |||
|
|
|||
| #### IN PROGRESS - v0.18.0 - _jdilly_, _fscarlier_, _fesoubel_ | |||
There was a problem hiding this comment.
Are we waiting for more to release? The Kmod branch?
| @@ -481,8 +481,10 @@ def _create_model_and_write_diff_to_measurements( | |||
| diff_columns = ( | |||
| list(OPTICS_PARAMS_CHOICES[:-4]) + | |||
| [col for col in corr_model_elements.columns if col.startswith("F1")] + | |||
| list(PLANES) | |||
| list(PLANES) + | |||
| ['MUX', 'MUY'] | |||
There was a problem hiding this comment.
This could also probably be from constants
| @@ -66,6 +66,9 @@ def correct(accel_inst: Accelerator, opt: DotDict) -> None: | |||
| meas_dict = filters.filter_measurement(optics_params, meas_dict, nominal_model, opt) | |||
| meas_dict = model_appenders.add_differences_to_model_to_measurements(nominal_model, meas_dict) | |||
|
|
|||
| if opt.arc_by_arc_phase and accel_inst.NAME == 'lhc': | |||
There was a problem hiding this comment.
I would go with Lhc.NAME but yeah, hardcoded to filter LHC
| model.loc[meas.index.to_numpy(), model_column].to_numpy()) | ||
| model_phases_advances = (model.loc[meas["NAME2"].to_numpy(), model_column].to_numpy() - | ||
| model.loc[meas.index.to_numpy(), model_column].to_numpy()) | ||
| model_phases_advances[model_phases_advances < 0] += tunes[plane] |
There was a problem hiding this comment.
This is probably a settingswithcopywarning, this PR could be the opportunity to use .loc and future proof a bit.
| help="Set to perform arc-by-arc total phase correction.", ) | ||
| params.add_parameter(name="include_ips_in_arc_by_arc", | ||
| type=str, | ||
| choices=("left", "right"), |
There was a problem hiding this comment.
Are the choices correct here since the default is None?
There was a problem hiding this comment.
I think "None" is always a choice when not required. I implemented this somewhen.
| excitation_name = "AC-Dipole" | ||
| possible_bpms = [f"BPMY{a_b}.6L4.B{beam}", f"BPM.7L4.B{beam}"] | ||
| element = f"MKQA.6L4.B{beam}" | ||
| elif self.excitation == AccExcitationMode.ADT: | ||
| excitation_name = "ADT-AC-Dipole" | ||
| possible_bpms = [f"BPMWA.B5{l_r}4.B{beam}", f"BPMWA.A5{l_r}4.B{beam}"] | ||
| element = f"ADTK{adt}5{l_r}4.B{beam}" | ||
| else: | ||
| return None | ||
|
|
||
| try: | ||
| return _first_one_in(possible_bpms, commonbpms), element |
There was a problem hiding this comment.
Better, thanks. Could you take the opportunity to write a mini docstring so we have written somewhere that this does not return the exciter BPM, but the exciter BPM and the exciter element name?
|
Code Climate has analyzed commit a34f920 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 44.3% (50% is the threshold). This pull request will bring the total coverage in the repository to 85.3% (-0.6% change). View more on Code Climate. |
It's all in a single pull request, since it was all entangled anyway:
changes:
Two things: