-
Notifications
You must be signed in to change notification settings - Fork 50
Initial refactoring #119
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
base: master
Are you sure you want to change the base?
Initial refactoring #119
Changes from all commits
d762040
d9b1f02
91cb71d
4a3be1c
de2fa99
3977887
b9b1243
26a9d35
01327a0
a44eabc
20eaddc
a70d321
c174bfe
419694b
ace2835
839b47b
bf739c9
b6755f7
5d93959
7f8f04e
1fa549e
2a3a1a4
32c971e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,6 @@ | |
"""Lyse analysis API | ||
""" | ||
|
||
from lyse.dataframe_utilities import get_series_from_shot as _get_singleshot | ||
from labscript_utils.dict_diff import dict_diff | ||
import os | ||
import socket | ||
import pickle as pickle | ||
|
@@ -25,7 +23,6 @@ | |
import contextlib | ||
|
||
import labscript_utils.h5_lock, h5py | ||
from labscript_utils.labconfig import LabConfig | ||
import pandas | ||
from numpy import array, ndarray, where | ||
import types | ||
|
@@ -34,9 +31,16 @@ | |
|
||
from labscript_utils import dedent | ||
from labscript_utils.ls_zprocess import zmq_get | ||
from labscript_utils.dict_diff import dict_diff | ||
|
||
from labscript_utils.properties import get_attributes, get_attribute, set_attributes | ||
LYSE_DIR = os.path.dirname(os.path.realpath(__file__)) | ||
|
||
# lyse imports | ||
import lyse.dataframe_utilities | ||
import lyse.utils | ||
|
||
# Import this way so LYSE_DIR is exposed when someone does import lyse or from lyse import * | ||
from lyse.utils import LYSE_DIR | ||
|
||
# If running stand-alone, and not from within lyse, the below two variables | ||
# will be as follows. Otherwise lyse will override them with spinning_top = | ||
|
@@ -55,13 +59,6 @@ | |
# a flag to determine whether we should wait for the delay event | ||
_delay_flag = False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These variables are causing quite a bit of grief for me. We have a decent number of places that need them across the modules, and I think it is generally considered a bad code smell to import from the module
I'm still trying to grok exactly how they are used and what their purpose is, as it isn't obvious to me why they are defined in this way. Most of them only appear to be necessary if you are using a script within the lyse GUI, and they wouldn't even be called with the plain API. I suspect I am unaware of some standard workflow. Do people commonly run their lyse scripts in the GUI, then also run them as stand-alone batch scripts unmodified (hence why Right now, I am inclined to move these global-like variables down to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spending a little more time with this, I don't see the purpose of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those are part of the next step of my Lyse changes that seem to have leaked into this refactor. Their function is associated with making the lyse plot windows "first class" windows whose positions are when lyse shuts down. This is the highest priority Lyse request in my group. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, though they appear to be on mainline already. Doesn't really matter where they came from I suppose, I'll plan on keeping them in. Would it be possible to implement figure position tracking independent of lyse globals? Chunking out distinct functionality, where feasible, goes a long way in ensuring these reviews don't drag on into absurdity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I have not visited this in about a year, I will use my old code as reference, but rewrite the implementation based on the current state of Lyse. |
||
|
||
# get port that lyse is using for communication | ||
try: | ||
_labconfig = LabConfig(required_params={"ports": ["lyse"]}) | ||
_lyse_port = int(_labconfig.get('ports', 'lyse')) | ||
except Exception: | ||
_lyse_port = 42519 | ||
|
||
if len(sys.argv) > 1: | ||
path = sys.argv[1] | ||
else: | ||
|
@@ -80,7 +77,7 @@ class _RoutineStorage(object): | |
routine_storage = _RoutineStorage() | ||
|
||
|
||
def data(filepath=None, host='localhost', port=_lyse_port, timeout=5, n_sequences=None, filter_kwargs=None): | ||
def data(filepath=None, host='localhost', port=lyse.utils.LYSE_PORT, timeout=5, n_sequences=None, filter_kwargs=None): | ||
"""Get data from the lyse dataframe or a file. | ||
|
||
This function allows for either extracting information from a run's hdf5 | ||
|
@@ -146,7 +143,7 @@ def data(filepath=None, host='localhost', port=_lyse_port, timeout=5, n_sequence | |
the lyse dataframe, or a subset of it, is returned. | ||
""" | ||
if filepath is not None: | ||
return _get_singleshot(filepath) | ||
return lyse.dataframe_utilities.get_series_from_shot(filepath) | ||
else: | ||
if n_sequences is not None: | ||
if not (type(n_sequences) is int and n_sequences >= 0): | ||
|
@@ -161,7 +158,7 @@ def data(filepath=None, host='localhost', port=_lyse_port, timeout=5, n_sequence | |
|
||
# Allow sending 'get dataframe' (without the enclosing list) if | ||
# n_sequences and filter_kwargs aren't provided. This is for backwards | ||
# compatability in case the server is running an outdated version of | ||
# compatibility in case the server is running an outdated version of | ||
# lyse. | ||
if n_sequences is None and filter_kwargs is None: | ||
command = 'get dataframe' | ||
|
@@ -178,35 +175,9 @@ def data(filepath=None, host='localhost', port=_lyse_port, timeout=5, n_sequence | |
raise ValueError(dedent(msg)) | ||
# Ensure conversion to multiindex is done, which needs to be done here | ||
# if the server is running an outdated version of lyse. | ||
_rangeindex_to_multiindex(df, inplace=True) | ||
lyse.dataframe_utilities.rangeindex_to_multiindex(df, inplace=True) | ||
return df | ||
|
||
def _rangeindex_to_multiindex(df, inplace): | ||
if isinstance(df.index, pandas.MultiIndex): | ||
# The dataframe has already been converted. | ||
return df | ||
try: | ||
padding = ('',)*(df.columns.nlevels - 1) | ||
try: | ||
integer_indexing = _labconfig.getboolean('lyse', 'integer_indexing') | ||
except (LabConfig.NoOptionError, LabConfig.NoSectionError): | ||
integer_indexing = False | ||
if integer_indexing: | ||
out = df.set_index(['sequence_index', 'run number', 'run repeat'], inplace=inplace, drop=False) | ||
# out is None if inplace is True, and is the new dataframe is inplace is False. | ||
if not inplace: | ||
df = out | ||
else: | ||
out = df.set_index([('sequence',) + padding,('run time',) + padding], inplace=inplace, drop=False) | ||
if not inplace: | ||
df = out | ||
df.index.names = ['sequence', 'run time'] | ||
except KeyError: | ||
# Empty DataFrame or index column not found, so fall back to RangeIndex instead | ||
pass | ||
df.sort_index(inplace=True) | ||
return df | ||
|
||
def globals_diff(run1, run2, group=None): | ||
"""Take a diff of the globals between two runs. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little bit hesitant about this
lyse.utils
import.lyse.utils
is importing Qt packages. But thelyse
package (e.g. this file) can be imported in either the GUI, or the worker process. We are probably tied to a Qt matplotlib backed in the worker process anyway? But it would be nice if we were not, or at least minimally so.One solution would be to make a
utils
dir, with three files (__init__.py
,gui.py
,worker.py
- or whatever names you like) so that the utils and imports can be split between only for gui, only for worker, or for both. That allows more specific imports to be made (note that the contents of the init file will be imported if either of the others are so that one should try to stay clean of Qt stuff as well).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a go at implementing this and did end up settling on your suggested divisions. All said and done, I think there is a decent argument for moving
dataframe_utilities
into the submodule or flattening the structure again and havingutils
,gui_utils
andworker_utils
at the top level. I am leaning towards the latter, but I think there is a more fundamental issue to fix first (I'll comment the code elsewhere) before sorting out stylistic preferences like this one.