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

Arc corr and glob check fix #437

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

Conversation

fscarlier
Copy link
Contributor

It's all in a single pull request, since it was all entangled anyway:

changes:

  • added arc-by-arc corrections
  • added the fixes we made together
  • changed the .json corrector files to include the mqt and ats only, and the 2024 global correction lists.
  • Added the arc-by-arc-phase and include-ips to the global_corrections parameters input

Two things:

  • Can you check if it is ok for the opt. definitions?
  • One test seems to fail for macos, but it doesn't seem to be related to the changes.

@fscarlier fscarlier requested a review from JoschD March 6, 2024 12:08
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.

I believe @JoschD has a few refactors & tests in mind. In the meantime, a few comments / suggestions.

A little bit of type hinting could go a long way for the new functions in omc3/correction/arc_by_arc.py

omc3/definitions/optics.py Show resolved Hide resolved
omc3/global_correction.py Outdated Show resolved Hide resolved
omc3/global_correction.py Outdated Show resolved Hide resolved


def get_arc_by_arc_bpm_pairs(meas_dict, opt):
bpms = meas_dict['PHASEX'].index
Copy link
Member

Choose a reason for hiding this comment

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

Could use the constants instead of the hard-coded PHASEX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I wasn't aware of the constants when I coded this.

Copy link
Member

Choose a reason for hiding this comment

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

The codebase is in an in-between with some hardcoded stuff still and some constants use, but for new code let's try to keep to the good way :P

@JoschD
Copy link
Member

JoschD commented Apr 22, 2024

Hey @fscarlier . This still needs tests, have you progressed in that regard?

@JoschD
Copy link
Member

JoschD commented May 8, 2024

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.

Copy link

codeclimate bot commented Jun 5, 2024

Code Climate has analyzed commit 51c2400 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

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

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

View more on Code Climate.

@JoschD
Copy link
Member

JoschD commented Jul 9, 2024

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?

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.

None yet

3 participants