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

Making ExitSlits subclass with FDQ component, only for SL1K2 for now #1307

Merged
merged 8 commits into from
Dec 12, 2024

Conversation

KaushikMalapati
Copy link
Contributor

@KaushikMalapati KaushikMalapati commented Nov 13, 2024

Description

ExitSlitsFDQ has an FDQ component so we can see flow meter readings on the typhos screen for SL1K2. I did not name this SL1K2 in case an ExitSlits+FDQ class is ever wanted in the future

Motivation and Context

https://jira.slac.stanford.edu/browse/ECS-6745

How Has This Been Tested?

Where Has This Been Documented?

image

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

@KaushikMalapati KaushikMalapati marked this pull request as ready for review November 13, 2024 02:13
@KaushikMalapati
Copy link
Contributor Author

@nrwslac with pcdshub/lcls-plc-rixs-optics#133 merged, can you please review this?

@KaushikMalapati
Copy link
Contributor Author

@ZLLentz could you look at this error log for the pip jobs?
https://github.com/pcdshub/pcdsdevices/actions/runs/12167562079/job/33936947300?pr=1307
I haven't changed anything that would affect attenuators, but it might have something to do with the test changes you made recently

@ZLLentz
Copy link
Member

ZLLentz commented Dec 5, 2024

could you look at this error log for the pip jobs
I haven't changed anything that would affect attenuators

I agree this has nothing to do with your code
I'll look a little more closely to see if I can understand what's going on, but I don't remember making test changes here recently. I might have modified the ci templates though...

pcdsdevices/slits.py Outdated Show resolved Hide resolved
@ZLLentz ZLLentz mentioned this pull request Dec 5, 2024
7 tasks
@KaushikMalapati
Copy link
Contributor Author

...but I don't remember making test changes here recently. I might have modified the ci templates though...
For some reason this file says it was changed 8 days ago but the last commit was on January 31
image

@ZLLentz
Copy link
Member

ZLLentz commented Dec 5, 2024

For some reason this file says it was changed 8 days ago but the last commit was on January 31

That's really bizarre. I feel less bad about not remembering a change from Jan/Feb than I do about not remembering a change from last week!

@ZLLentz
Copy link
Member

ZLLentz commented Dec 5, 2024

We can either merge #1311 first or merge this with bypassing the failing tests

@KaushikMalapati
Copy link
Contributor Author

We can either merge #1311 first or merge this with bypassing the failing tests

Let's wait for #1311

@ZLLentz
Copy link
Member

ZLLentz commented Dec 5, 2024

#1311 is merged so I'll click update branch here, anticipating green check marks all around

tangkong
tangkong previously approved these changes Dec 5, 2024
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

@KaushikMalapati
Copy link
Contributor Author

@nrwslac I added the doc string and think this is ready. If you think it's good, I will merge and update the device_class for sl1k2

@KaushikMalapati
Copy link
Contributor Author

Actually, I took a second look at this. Did you want the FDQ to be in ExitSlits directly instead of a subclass?

@KaushikMalapati
Copy link
Contributor Author

I decided to add the flow meter to ExitSlits directly. sl1k2 is the only device that uses the class so it didn't make sense to me to subclass it.

Copy link
Contributor

@tangkong tangkong 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 you are correct. Currently SL1K2 is the only consumer of this class. This does make the requirements for implementing this class more strict in the future, but we can cross that bridge when we get there (perhaps this motivates us to add these flow monitors, in a backwards sort of way)

@KaushikMalapati KaushikMalapati merged commit e029cbc into pcdshub:master Dec 12, 2024
11 checks passed
@KaushikMalapati KaushikMalapati deleted the sl1k2 branch December 12, 2024 17:57
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.

4 participants