adding too and refactoring screen class

slaclab / lcls-tools

Python tools for LCLS: post processing data, PV handling, pulling archive settings, etc.

Apache License 2.0

2 stars 20 forks source link

adding too and refactoring screen class #128

Open roussel-ryan opened 7 months ago

roussel-ryan commented 7 months ago

This PR does the following: These are major changes so I'm certainly looking for feedback on how you want to do this:

splits the functionality of the screen class into two, a device object that represents the screen object along with associated properties, and a ScreenProfileMeasurement object that combines the device info with the fitting tool to create a measurement.
moves saving images and metadata to measurement class since these specifications are relevant more to the measurement process not the hardware object (I think we can have more discussion about this)
adds a method to the screen object that can measure a background and save that info to file, incl. methods to shutter the beam
removes some of the timeout logic because it doesn't seem necessary since the operations are blocking, we could also move this logic to the PV class -- there might be something that I'm missing that you guys identified earlier
adds a yaml env file

nneveu commented 7 months ago

Can you run flake8 and black? the formatting checks are failing and I'd like to see if the tests pass.

roussel-ryan commented 7 months ago

I'd like to get general feedback on the overall structure of what I'm proposing before fixing all of the tests etc if possible, then I'd be happy to fine tune w.r.t. formatting

nneveu commented 7 months ago

Lots of great work, thanks Ryan! Let's chat before or at the SWS meeting. I think raw measurements should stay with the device class and any post processing should be elsewhere. It's definitely up for discussion though! Happy to hear what @eloiseyang @shamin-slac @MattKing06 @kabanaty think too.

roussel-ryan commented 7 months ago

Sounds good, I'm happy to discuss on Monday

eloiseyang commented 7 months ago

Can you run flake8 and black? the formatting checks are failing and I'd like to see if the tests pass.

@nneveu We should set up separate workflows for style testing and unit testing. That way the unit tests won't get blocked by a failing style test, as they are right now.

edit: Actually, maybe not, since flake8 also looks for python syntax errors. Right now we're blocked on some unused variables, and it would still be nice to know if the unit tests pass, since those syntax errors aren't critical.

shamin-slac commented 7 months ago

Lots of great work, thanks Ryan! Let's chat before or at the SWS meeting. I think raw measurements should stay with the device class and any post processing should be elsewhere. It's definitely up for discussion though! Happy to hear what @eloiseyang @shamin-slac @MattKing06 @kabanaty think too.

I agree with this. I think measurements and processing should be separate. It also might be worth considering separating the device class that has properties from the methods that interact with EPICS.

kabanaty commented 7 months ago

Lots of great work, thanks Ryan! Let's chat before or at the SWS meeting. I think raw measurements should stay with the device class and any post processing should be elsewhere. It's definitely up for discussion though! Happy to hear what @eloiseyang @shamin-slac @MattKing06 @kabanaty think too.

I agree that separation between raw measurements and processed data is a good thing. I'll wait to give too much feedback since I'm not fully up to speed on lcls-tools. Looking forward to SWS on Monday and asking some more questions. Thanks!