Finally merging #85

[dihm]

This PR resolves all the merge conflicts of #85 so it can finally be merged.

[dihm]

Note: need to do a little testing to make sure the merge was done correctly, especially around the open_file decorator and context_manager functionality.

[dihm]

I have confirmed that the merge hasn't broken anything and the FutureWarning shows.

Default python behavior has the warning only showing on first run of an analysis worker only. That may make the warning easy to miss for groups who don't restart lyse often. It's an easy fix to make it always show, but want to be sure that is what we want to do before constantly annoying a bunch of people to update.

[ispielma]

Does the warning show in the OutputBox? If so could it be made to show each time the worker is activated? This would make it visible from time to time without being really annoying. I was meaning to add a set activate\inactive message to the worker processes anyway so they can change the state of their windows.

[dihm]

The warning does show in the output box. There are a few potential ways to show at each activation, but I don't super love them.

1. Have the worker process reset the warnings registry (calling warnings.resetwarnings()). Effective, but has a global effect as it would reset all warnings for the worker, and potentially even outside the worker (would need to test this).
2. We could manually track activation and selectively catch the warning with the catch_warnings context manager. This somewhat negates a lot of the benefits of using warnings in the first place given how manual it is. I'd just as soon live with only showing the warning once.
3. There is likely some very subtle method that muxes with the filter registry at a lower level, but I wouldn't want to support that for such a small benefit.

[dihm]

Going back through this, I am now realizing that the standard lyse dataframe column labels also include run everywhere (ie run time, run number, run repeat, n_runs). Fixing this in a backwards compatible way is going to be a moderate pain, especially with respect to reloading saved dataframes. It is also going to lead to more broken user code since folks are more likely to be using these columns names in their analysis scripts.

@philipstarkey I am highly tempted to make that step a separate PR since it is more likely to break things than this one is. Do you have a preference? Maybe you already made the choice to not touch those things and I should just respect that?

[dihm]

The more I look at this, the less convinced I am that it is worth the trouble. run time, run number, run repeat and n_runs are all defined are saved attributes in the h5 file, set by runmanager. I'm not convinced that changing lyse.Run to lyse.Shot makes much sense if we don't also change these underlying names that are ostensibly used by user code. And changing them would involve a semi-large incompatible change to a lot of code since I don't really have any desire to support both in some backwards compatible way.

As much as I prefer the paradigm of shot is a single execution in BLACS, sequence is a collection of shots, and run=engage in runmanager and therefore could be either; I'm more inclined to go through and carefully scrub out shots in favor of runs to ensure some consistency while saving us a headache.

@philipstarkey (or others) I'm willing to be persuaded otherwise, but I think this should be rejected (or at least saved for labscript v4 that may or may not happen)

[philipstarkey]

Yep it's been 4+ years, I'm happy to abandon it if you don't think it's worth it.

[ispielma]

What I would propose is a revision of this that keeps the old syntax AND the new syntax. Then, as is typical for this sort of thing, throws a warning that the old syntax is depreciated and will be dropped in a future version. Ian B. Spielman Fellow, Joint Quantum Institute National Institute of Standards and Technology and the University of Maryland

…

----- WEB ----- http://ultracold.jqi.umd.edu

----- EMAIL ----- ***@***.***

----- ZOOM ----- https://umd.zoom.us/j/7984811536

----- MAIL ----- UMD: 2207 Computer & Space Sciences Bldg. College Park, MD 20742 NIST: 100 Bureau Drive, Stop 8424 Gaithersburg, MD 20899-8424 USA

----- OFFICE ----- UMD: Physical Sciences Complex, Room 2153 NIST: Building 216, Room B131

On Apr 11, 2025, at 20:59, Phil Starkey ***@***.***> wrote: Yep it's been 4+ years, I'm happy to abandon it if you don't think it's worth it. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***> philipstarkey left a comment (labscript-suite/lyse#118) Yep it's been 4+ years, I'm happy to abandon it if you don't think it's worth it. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

[dihm]

I did think for a bit on how to backwards compatible support this change. The rub is that these underlying run * names are columns in the lyse dataframe and therefore directly user facing on a data structure we don't own. A backwards compatibility layer that intercepts the pandas dataframe and hacks on column-name translation is well beyond what I want to maintain. Especially when I'm looking down a Qt5 deprecation, code-base refactors, and trying to actually add improvements.

As far as I can tell, the only reasonable way to support both would be simply duplicating the columns (ie have a run number and an identical shot number). We could then go through the rest of the suite at our leisure and slowly convert everything (by first duplicating in the h5 files, then in the BLACS handling, etc). It is a reasonable, if slightly wasteful, way forward, but ultimately I'm not sure the semantic moral victory is worth the effort right now.