Improve Qmini spectrometer GUI #1314

aberges-SLAC · 2024-12-19T22:57:53Z

Description

Gave a face-lift to the Qmini embedded & (new) detailed screen(s) to better suit the needs of SRD in MODS.

Motivation and Context

Adds a shiny new save-spectrum feature so that scientists aren't tempted to disconnect the spectrometers and plug it into their laptops ;]

How Has This Been Tested?

I've tested this in the pcds-5.9.1 environment and pretty extensively:
https://github.com/user-attachments/assets/fb2ba094-2165-40a2-9e9b-bd6075e3bb4e

Where Has This Been Documented?

In this PR and in the code!

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

…ded qmini screen

ZLLentz

I like what you've done here in the python file and I mostly have nitpicks and questions
I'll peek at the ui files next

ZLLentz · 2024-12-20T00:30:38Z

pcdsdevices/lasers/qmini.py

@@ -83,6 +87,47 @@ class QminiSpectrometer(Device):
    fit_stdev = Cpt(EpicsSignalRO, ':STDEV', kind='config')
    fit_chisq = Cpt(EpicsSignalRO, ':CHISQ', kind='config')

+    # Save spectra functions
+    def save_data(self):


Nitpick, can ignore:
Is there any utility in making this more useful in non-gui contexts?

def save_data(self, filename: str = ""): if not filename: filename = self.file_dest.get().strip()

What did you have in mind?

In a non-gui context you could just do the shell script method of:

# save-spectrum.sh or whatever we wanna call it file_dest=#2 prefix=#1 caget -t $prefix:WAVELENGTHS $prefix:SPECTRUM >> file_dest.txt

With a call like save-spectrum SOME:BASE:PV:SPECTROMETER save_me_to_some_file.txt

However there is this weird bug with the caget -t call were the first element of the array is actually the array size, so you always get 2018 as the first element in both x & y. 😕

I'm imagining me having a qmini object in hutch-python for some reason
Then, I want to save the data
I call

qmini.save_data()

But then since I don't have the GUI open it's not obvious that I'm supposed to put to a string signal.

So maybe it would be nice if I could also do:

qmini.save_data("some/path/of/mine")

ZLLentz · 2024-12-20T00:33:02Z

pcdsdevices/lasers/qmini.py

+        """
+        if len(self.file_dest.get().strip()) == 0:
+            # set a default destination for the file if you're lazy or give it whitespace
+            _file = os.getcwd() + '/' + self.name + time.strftime("_%Y-%m-%d_%H%M%S") + '.txt'


Question: I noticed that you prepend a lot of function-scoped variables with a leading underscore. I'd previously only seen this as a convention for semi-private class members and function names that are intended not to be exported via import as per https://peps.python.org/pep-0008/.

Are there other reasons to name variables like this?

I'm doing for (paranoid) privacy reasons on the calls. I'm probably being overly cautious, honestly, but it's a habit I've kept. I don't think there's going to be any conflicts (aside from the signal attribute name shenanigans that need it — as I found out the hard way)

Can you expand a bit on privacy reasons? My question is mostly to learn

I think, strictly speaking, _var is functionally a stylistic convention for saying "this var is private-ish, but I'm not gonna enforce it. Remember that you need to be janky to get here" vs. __var getting the built-in string mangling.

Mostly useful for me in contexts where there are pretty commonly recycled var names (i.e. result , data, output, etc.) and I want to be explicit about which one I am messing with. That way I can have a parent function return a result dependent on an inner function call with output _result without getting too weird. Idk maybe I inherited some weird jank convention from C that I haven't unlearned.

Gotcha, I do the same but only for data attributes e.g. self._var. I can see how it'd also be nice to indicate which variables are meant to persist through the function and which are e.g. loop-only. Thanks for humoring me.

ZLLentz · 2024-12-20T00:34:19Z

pcdsdevices/lasers/qmini.py

+            # set a default destination for the file if you're lazy or give it whitespace
+            _file = os.getcwd() + '/' + self.name + time.strftime("_%Y-%m-%d_%H%M%S") + '.txt'
+        else:
+            _file = self.file_dest.get()


A few lines above you strip the text contents from this signal. Maybe you should strip here too to help users avoid having leading/trailing spaces in their filenames.

pcdsdevices/lasers/qmini.py

ZLLentz · 2024-12-20T00:40:10Z

pcdsdevices/lasers/qmini.py

+                 'exposure (us)': str(self.exposure.get()),
+                 'averages': str(self.exposures_to_average.get()),
+                 # Lets do some sneaky conversion to bool from int
+                 'settings': {f"{sig}": str(eval(f"{self}.{sig}.get() > 0")).lower()


Nitpick: maybe simpler without an eval so the intent is clearer and the next maintainer doesn't get confused.
I probably botched this because I also got confused. I left it as an int because ints are allowed in json.

Suggested change

'settings': {f"{sig}": str(eval(f"{self}.{sig}.get() > 0")).lower()

'settings': {f"{sig}": int(getattr(self, sig).get())

I neglect getattr a little too often for my own good, that's much cleaner. int are allowed but I preferred true and false outputs for the states to match their respective PVs as bo records.

This is maybe just convenience for the consumer of the dump, since it saves them a comparison when checking settings in the object.

I might just be enforcing my own preferences, tbh, so it isn't strictly necessary. Maybe I'll ask the scientists what they prefer 🤷

Gotcha, I guess since the point is for the end-user to read there could a lot of ways to do it!

You can also put boolean literals into json and it looks like true or false without the quotes.

I guess I could've just cast to bool anyway, right? Not sure why I compared to get there in the first place lol. Very good point, I like that much better and will do that

ZLLentz · 2024-12-20T00:40:47Z

pcdsdevices/lasers/qmini.py

+                     'normalize_exposure', 'adjust_offset', 'subtract_dark',
+                     'remove_bad_pixels', 'remove_temp_bad_pixels']
+        _data = {'timestamp': time.strftime("%Y-%m-%d %H:%M:%S"),
+                 'exposure (us)': str(self.exposure.get()),


I'm not sure I understand why these need to be cast to strings.

json.dumps was being weird for a second and I think I cast everything to a str when sorting out the problem and left that in there. These can probably live as the ints that they are

Ah, but the wavelength & spectra must be cast to str since Float32 is not json compat, but that's on the consumer to handle turning back into a number.

ZLLentz · 2024-12-20T00:46:28Z

pcdsdevices/ui/QminiSpectrometer.detailed.ui

UI files look good!

Note that there's one small discrepancy: the embedded has a button with the text "Start Single Exposure" and the detailed has the same button with the text "Force Single Exposure". I don't have any preference but maybe the text should be the same if they do the same thing!

Oooooh very good catch! I meant for these to both be "Force" to be consistent with the typhos default detailed screen that the folks are used to.

ZLLentz · 2024-12-20T00:47:07Z

I think this is mergeable with consideration or rejection of my comments + a pre-release docs page

Co-authored-by: Zachary Lentz <[email protected]>

…que use

ZLLentz

I think we should bring this in!

ZLLentz · 2024-12-20T18:13:15Z

I'm not sure if you're online Adam, so I'm going to merge this myself to include it in the imminent tag!

aberges-SLAC and others added 4 commits December 19, 2024 13:59

ENH: various improvements to arrangement, layout, and display of embe…

f9f78d0

…ded qmini screen

ENH: Add a new qmini detailed screen

64266bf

ENH: Add func to save spectrum to disk and signal for choosing file dest

1dc4320

Merge branch 'pcdshub:master' into qmini_gui_update

cbcecee

aberges-SLAC requested a review from ZLLentz December 19, 2024 23:14

ZLLentz reviewed Dec 20, 2024

View reviewed changes

aberges-SLAC and others added 3 commits December 19, 2024 16:48

MNT: better check logic for empty file_dest values

301c7d6

Co-authored-by: Zachary Lentz <[email protected]>

MNT: fix text on "Force Exposure" button

9b55c16

MNT: Fix nitpicks and generalize save_date method for hutch-python-es…

416e088

…que use

aberges-SLAC requested a review from ZLLentz December 20, 2024 02:27

ENH: add release notes for 1313

1a65dd5

ZLLentz mentioned this pull request Dec 20, 2024

REL: release notes v8.7.0 #1315

Merged

ZLLentz approved these changes Dec 20, 2024

View reviewed changes

ZLLentz merged commit bf311ec into pcdshub:master Dec 20, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Qmini spectrometer GUI #1314

Improve Qmini spectrometer GUI #1314

aberges-SLAC commented Dec 19, 2024

ZLLentz left a comment

ZLLentz Dec 20, 2024

aberges-SLAC Dec 20, 2024 •

edited

Loading

ZLLentz Dec 20, 2024

ZLLentz Dec 20, 2024

aberges-SLAC Dec 20, 2024

ZLLentz Dec 20, 2024

aberges-SLAC Dec 20, 2024

ZLLentz Dec 20, 2024

ZLLentz Dec 20, 2024

ZLLentz Dec 20, 2024

aberges-SLAC Dec 20, 2024

ZLLentz Dec 20, 2024 •

edited

Loading

aberges-SLAC Dec 20, 2024

ZLLentz Dec 20, 2024

aberges-SLAC Dec 20, 2024

aberges-SLAC Dec 20, 2024

ZLLentz Dec 20, 2024

aberges-SLAC Dec 20, 2024

ZLLentz commented Dec 20, 2024

ZLLentz left a comment

ZLLentz commented Dec 20, 2024

	'settings': {f"{sig}": str(eval(f"{self}.{sig}.get() > 0")).lower()
	'settings': {f"{sig}": int(getattr(self, sig).get())

Improve Qmini spectrometer GUI #1314

Improve Qmini spectrometer GUI #1314

Conversation

aberges-SLAC commented Dec 19, 2024

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Pre-merge checklist

ZLLentz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aberges-SLAC Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz commented Dec 20, 2024

ZLLentz left a comment

Choose a reason for hiding this comment

ZLLentz commented Dec 20, 2024

aberges-SLAC Dec 20, 2024 •

edited

Loading

ZLLentz Dec 20, 2024 •

edited

Loading