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

Add ACR status option for CCM energy moves #1194

Merged
merged 14 commits into from
Mar 8, 2024

Conversation

vespos
Copy link
Contributor

@vespos vespos commented Jan 22, 2024

Description

Add the ACR status PV for cases where we are moving the undulator or the K, which need to be waited on.

Motivation and Context

How Has This Been Tested?

In XPP3, with ACR changing the K value.

Where Has This Been Documented?

N/A

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@vespos vespos requested a review from tangkong January 22, 2024 00:30
For HXR this usually is 'AO805'.
"""
vernier = FCpt(BeamEnergyRequest, '{hutch}',
acr_status_suffix='AO805',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I use the acr_status_suffix here instead of hardcoding the 'AO805'?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like FormattedComponent only allows you to format the suffix. DynamicDeviceComponent also doesn't quite fit the bill. I don't see a Component type in ophyd or pcdsdevices that lets you format an arbitrary kwarg, but I feel like we must have done something like this before.
Maybe @ZLLentz has better memory than I do?

Copy link
Member

Choose a reason for hiding this comment

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

There's a argument from Component called add_prefix that defaults to ("suffix", "write_pv"), which is used to determine which string arguments we should prepend with the prefix. This is re-used in FormattedComponent to determine which string arguments should be formatted. So, you could do:

    vernier = FCpt(BeamEnergyRequest, '{hutch}',
                   acr_status_suffix='{acr_suffix}',
                   add_prefix=('suffix', 'acr_status_suffix')

    def __init__(self, *args, **kwargs):
        self.acr_suffix = 'AO805'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks, that's a neat backdoor. (or at least something I've never seen in the Component class)

@vespos
Copy link
Contributor Author

vespos commented Feb 27, 2024

@ZLLentz @tangkong I have pushed some more changes. We are going to test things this week with ACR so there will probably be more to come.

I changed some naming, as to better reflect the reality of the use cases. While i feel this is making things less confusing in the long run, I don't know if that is not breaking things elsewhere. Can you advise? The changes are:

  • vernier -> acr_energy: the energy may or may not be the Vernier. This is particularly true for the case when we wait for the acr status
  • bunch -> pv_index: we are using the different PVs not only for multi-color / multi bunch, but also like here for combined Vernier and K motion

@tangkong
Copy link
Contributor

Recording what we discussed briefly on slack:

  • the kwarg change is definitely breaking
  • a quick search through pcdsdevices shows only the CCM using BeamRequestEnergy classes
  • a quick grep through the hutch-python directories show only TMO using BeamRequestEnergy classes
  • A making the kwarg change back compatible is a simple enough way to remedy our issues

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

This is tested working live, right? I think it looks good. The vernier name change is still an api break but it's definitely more correct and probably ok.

This is just missing pre-release docs I think.

@vespos
Copy link
Contributor Author

vespos commented Feb 27, 2024

Addressed the comments. I am also working on a confluence page to go more in depth about the energy scanning schemes that we are using.
Next step for after the actual tests with ACR.

@tangkong tangkong mentioned this pull request Feb 28, 2024
7 tasks
@vespos
Copy link
Contributor Author

vespos commented Mar 6, 2024

So we did some self-seeding test recently, and I have added a couple commits related to that.
Pending the test fix and the release notes, I think we are close to being able to merge.

Two things that I would like your input on:

  • Was there any reason to have the Vernier tolerance set to 5eV? I assume this is only driven by the SASE beandwidth (~10eV), and it needs to be reduced to 0.5eV for the self-seed (bandwidth ~1eV). But is there any other consideration to take into account?
  • Are we ok with having the reargs function live in the ccm.py? Do we want to have that as a most consistent way to re-arg functions should we need that?

@ZLLentz
Copy link
Member

ZLLentz commented Mar 6, 2024

Was there any reason to have the Vernier tolerance set to 5eV?

I had remembered this being inherited from an older version of the code, but when I went to check I found that the original source used 3eV. My limited understanding is that if the move is small enough we don't want to wait for the energy to change, which can be a timely process.

Are we ok with having the reargs function live in the ccm.py?

It probably belongs in a utils module instead, but it could always be moved in a later refactor if needed.

@vespos
Copy link
Contributor Author

vespos commented Mar 6, 2024

Ok, so moving to 0.5eV is fine. The Vernier change takes 0.2-0.3s, so it's comparable if not faster than the CCM.
It's been tested with beam, and in the self-seed case, the 5eV tolerance definitely breaks things. In SASE, the Vernier move is not a bottleneck.

I'll move the reargs function to the utils.py here, it makes indeed more sense to have it there.

@vespos vespos requested review from tangkong and ZLLentz March 7, 2024 01:06
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

This looks good to me

@vespos vespos merged commit 61fdf11 into pcdshub:master Mar 8, 2024
9 checks passed
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