Skip to content

Fix sign issue for horizontal kick angle#84

Merged
JeanLucPons merged 6 commits intomainfrom
fix-sign-for-horz-kick
Nov 25, 2025
Merged

Fix sign issue for horizontal kick angle#84
JeanLucPons merged 6 commits intomainfrom
fix-sign-for-horz-kick

Conversation

@JeanLucPons
Copy link
Copy Markdown
Contributor

@JeanLucPons JeanLucPons commented Nov 24, 2025

This PR solves sign issue for horizontal kick angle convention.
See #82

@gubaidulinvadim
Copy link
Copy Markdown
Contributor

It looks good, but the sign convention should be specified in the configuration file, not in the source code.

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

JeanLucPons commented Nov 24, 2025

As it is possible to load several accelerators, that means if the convention is inside the config, both convention can be blended.
It is possible to implemented in this way (i do not recommend) and it will require much modifications in the source.

@kparasch
Copy link
Copy Markdown
Contributor

I think it's ok for now, but eventually I believe we should pick a convention and stick with it. Then it would be the job of the magnet models in the configurations to have correct conversions.

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

Yes i would be in favor on fixing one.

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

But i also think that it is OK for the moment.
Discussion around convention will take ages !

@gubaidulinvadim
Copy link
Copy Markdown
Contributor

Ok, it's fine for me like that.

"""
Set the kick angle.
"""
return self.ange
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.

Should this be self.__angle?

"""
Set the kick angle.
"""
return self.ange
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.

Same here, should this be self.__angle?

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

I'm the king of the typos :D
So you can imagine how difficult is python for me !

@gubaidulinvadim
Copy link
Copy Markdown
Contributor

FYI, i've merged the formatting changes to the main. So, this merge request is broken because of that. There are a few easy merge conflicts to resolve.

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

Ok I will solve conflicts tomorrow.
Thanks to you

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

@gubaidulinvadim
I have an issue with pre-commit.
My branch is well rebased on main.
Unfortunately during the commit, i forget to reenable pre-commit by pre-commit install.
So i did a pre-commit run --all-files
I would expect change only in my files but i got a long list instead ?!

commit 013943f127bd8bebb4a85182cf47afe624ec7895 (HEAD -> fix-sign-for-horz-kick, origin/fix-sign-for-horz-kick)
Author: PONS <pons@esrf.fr>
Date:   Tue Nov 25 09:03:13 2025 +0100

    Fix typos, added test for kick angle

commit 53e18b8df07b003505c8f0d8ef1738965f82b392
Author: PONS <pons@esrf.fr>
Date:   Mon Nov 24 14:03:24 2025 +0100

    Added missing file

commit c06160e0b25290a3758e8c775fb6a0d1a6ccc3ec
Author: PONS <pons@esrf.fr>
Date:   Mon Nov 24 14:02:43 2025 +0100

    Configurable horizontal kick angle convention

commit 5434fe9906056b762c907641195fdf0c72053c6f
Author: PONS <pons@esrf.fr>
Date:   Mon Nov 24 09:00:04 2025 +0100

    Fix sign issue for horizontal kick angle

commit 5462d452b062b013281b927f63107e6176abc343 (origin/main, origin/HEAD, main)
Merge: 3611755 4bc9531
Author: Vadim Gubaidulin <gubaidulinvadim@gmail.com>
Date:   Mon Nov 24 17:31:04 2025 +0100

    Merge pull request #79 from python-accelerator-middle-layer/project-formatting
    
    Added formatting and precommit hook

@gubaidulinvadim
Copy link
Copy Markdown
Contributor

gubaidulinvadim commented Nov 25, 2025

@JeanLucPons , did you fix your issue? When I run locally the pre-commit I had no issue just right now. It only fixed the trailing white spaces for me. And it was only in docs/, tests/ and .github/ folders. Those are maybe not included in the actual pre-commit hook and CI/CD. I'm not sure.

Edit: failed ruff CI/CD pipeline from your commits above only has vcorrector.py and hcorrector.py.

@JeanLucPons JeanLucPons merged commit 9829e79 into main Nov 25, 2025
3 checks passed
@gubaidulinvadim gubaidulinvadim deleted the fix-sign-for-horz-kick branch November 25, 2025 10:44
@JeanLucPons
Copy link
Copy Markdown
Contributor Author

@gubaidulinvadim
Here is the list of modified files after my pre-commit run --all-files.
I only committed files that I modified and that was failing during the ruff check.
I'm not an expert with pre-commit and I don't know what to do in case of you commit to your local branch without pre-commit enabled. May be i should not have use --all-files option

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   .github/ISSUE_TEMPLATE/feature_request.md
        modified:   .github/PULL_REQUEST_TEMPLATE/pull_request_template.md
        modified:   .github/workflows/deploy-pypi.yaml
        modified:   .github/workflows/documentation.yml
        modified:   .gitignore
        modified:   .readthedocs.yaml
        modified:   README.md
        modified:   docs/Makefile
        modified:   docs/api.rst
        modified:   docs/index.rst
        modified:   docs/modules/bpm.rst
        modified:   docs/modules/configuration.rst
        modified:   docs/modules/lattice.rst
        modified:   docs/modules/pyaml.rst
        modified:   docs/modules/rf.rst
        modified:   docs/requirements.txt
        modified:   tests/config/EBSOrbit.yaml
        modified:   tests/config/EBSTune.yaml
        modified:   tests/config/EBS_rf.yaml
        modified:   tests/config/EBS_rf_multi.yaml
        modified:   tests/config/EBS_rf_notrans.yaml
        modified:   tests/config/bad_conf_cycles.json
        modified:   tests/config/bpms.yaml
        modified:   tests/config/sr-attribute-linker.yaml
        modified:   tests/config/sr-ident-cfm.yaml
        modified:   tests/config/sr/correctors/SH1AC01-ident.yaml
        modified:   tests/config/sr/correctors/SH1AC01.yaml
        modified:   tests/config/sr/correctors/SH1AC02.yaml
        modified:   tests/config/sr/custom_magnets/hidcorr.yaml
        modified:   tests/config/sr/magnet_models/QD2_strength.csv
        modified:   tests/config/sr/magnet_models/QF1AC01.yaml
        modified:   tests/config/sr/magnet_models/SH1AC01.yaml
        modified:   tests/config/sr/magnet_models/SH1AC02.yaml
        modified:   tests/config/sr/magnet_models/quadcurve.yaml
        modified:   tests/config/sr/magnet_models/quadcurve_data.yaml
        modified:   tests/config/sr/quadrupoles/QF1AC01-IDENT-HW.yaml
        modified:   tests/config/sr/quadrupoles/QF1C01A_2.yaml
        modified:   tests/external/pyproject.toml

@gubaidulinvadim
Copy link
Copy Markdown
Contributor

Strange, maybe docs/ and tests/ folders were excluded when I did the formatting. I cannot see where the issue is. But I had to rerun the formatting (e064bf9) on this same list of files, too.

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.

3 participants