Point on refactoring

[SamuelDeleglise]

Current status

I am now at the point where everything is working fine again: the whole attribute layer has been carefully redesigned to include gui and parameter saving in an efficient and elegant way. All unittests are OK, and for the end-user, the API has only undergone minor changes:

• accessing software modules (such as na and spectrum_analyzer) is done via pyrpl.na.
• accessing hardware_module is done via either the old way (pyrpl.rp.iq1 for quick debugging) or using the ModuleManagers (iq = pyrpl.iqs.pop(owner_name) to be consistent, scope and iir can also be accessed via pyrpl.scopes.pop() or pyrpl.iir.pop(), after all, it's only an implementation detail that we have decided to have only one scope and one iir...)
  • The whole organization of the config file has been redesigned: it is now pretty much module-oriented (one module=one section)

New cool features:

• The attribute widgets are now working perfectly (even in log increment mode), because they are fully aware of the minimum increment and max/min values of the underlying attribute (I also included support for mouse wheel).
• Thanks to the normalized way attributes are defined, all buttons now come with an informative tooltip (help displayed upon mouse hover) with min/max/increment values. Also, the attribute's helpstring is displayed in the tooltip.
• Modules are auto-saving and reloading their parameters from the config file.
• setup() finally has a nice help string autogenerated from the setup_attributes list (the same that is used to know which parameters are necessary to save/store the module state). In the end the helpstring for attributes is defined only in one place (at the attribute's creation), and not duplicated in the setup() helpstring, which was a pain to maintain.
  • The config file defines a list of modules to be loaded at startup to make things modular. For instance, those only interested by interfacing the hardware_modules of the redpitaya (previously RedPitaya class users) would leave this list empty.
  • Another big advantage of the new attribute design is that creating new software_modules with automatically generated GUI and parameter saving is extremely easy (and clean). The plan is to allow users to build their own software_modules (or derived module classes) to control their experiment and to include these external modules without screwing up the repository by adding "my_external_python_package.MySoftwareModuleClass" in the list of modules defined in the config file.
• There is a gui for the iir module (to be improved, see below).
• There is an ownership mechanism for modules that is fully integrated with the pop() logic and parameter saving. For instance, when a module is freed, its attributes automatically come back to the preceding state. Also, the widget visibility is disabled/enabled when its ownership changes (the owner name is also visible for clarity).
• For instance, the scope<-> spec an interaction is much more peaceful than previously: launching a spec-an acquisition will stop the current scope acquisition, disable the scope gui for the time of the acquisition, and put back the scope in the previous working state afterwards. Now, there is no need to have scope and spec an guis on 2 mutually exclusive tabs.
• Possibility to close and reopen unneeded modules in the gui.
• I finally found the way to show a message (with nice coloring) in the statusbar of Pyrpl's main window when an exception occurs in the event loop (working both in ipython or standalone mode): this avoids the big pitfall of not noticing the exceptions when they pop out in the background console.

That's all I can think of right now...

Issues fixed:

• 76 IIR gui -->I would say half of it, see below

• 65 module params should be uniformized --> That was the initial point

• 40 Renaming of variables (the part of it on numbering) --> All module index are now 1-based

• 36 Gui bug collections: I expect most of these bugs to be gone, and given the much cleaner gui code now, bug hunting in the gui should become a pleasure.

What is still left to do before pulling into master:

• Make some new unittests to test the new functionalities.

  • Gui for the iir is still minimalist (basically, an interface to define zeros and poles), it would be really nice to include the graph with the transfer function in the widget, with some nice keyboard shortcuts to move zeros/poles values as you (Leo) did it in bodeplot. However, I am not sure if I will be very efficient at doing that, and I will probably wait for you before doing it since I am not sure if we want to just keep everything from your bodefit or to adapt it slightly (for instance, using pyqtgraph and QShortCuts instead of matplotlib?).

• With this huge cleanup in attribute saving, it becomes extremely easy to add save/restore states for each modules. for instance, if a new state for an iq is saved with name 'pdh', I suggest to create a section "iq__pdh" (2 underscores to avoid accidental collision with user module names) in the config file, that can be loaded to any of the 3 iq modules later on. Of course, for software modules, this will work perfectly fine as well since they use the same Attribute logic. Even cooler, we could choose to let the attribute save their modifications in the "active" state of each modules instead of the default state (eventually, there could be a flag "autosave=True/False" in each state).

• Lockbox is still not re-included in the code (and that is probably the most important point of this post where I would need feedback): my plan is to encapsulate the lockbox functionality inside a software_module "lockbox" that would be accessed similar to the na or the spectrum_analyzer. I guess it would be nice to build a little bit upon the load/save state mechanism that I described above. For instance, it would make sense that when going into sweep mode, rather than having the lockbox slaving the scope (and thus blocking any gui input), to have the lockbox load the "sweep" state of the scope, allowing the user to make modifications on that state on the fly. The only time when the scope needs to be really slaved is when acquiring calibration curves (anyway, the "sweep" state could be used) Also since I believe each OutputSignal has an associated pid, why not to have a state "fast_piezo", "slow_piezo" (the output signal names) for the pids. These states would be loaded whenever the lockbox is set to a locking state (actually, in this case, we probably also want the iqs to be slaved because gains are really handled by the high-level lockbox module).

A possibility would be that the scope, iq, and iir states are only created once by the lockbox (and then eventually manipulated by the adult user) while the pids states are always overwritten to match the PI corner frequencies and other internal attributes of the lockbox (or maybe even better, not using states at all for the pids).

Long term evolution:

• A CurveManager SoftwareModule replacing pyinstruments would definitely fit very nicely in this modular architecture. In this case, there are a lot of design mistakes that we could avoid with our experience of pyinstruments. Would we keep django for the database model?

[lneuhaus]

issue #81 summary (closed and printed here since it is related):

1. Registers handle their gui themselves: The Registers have a function create_widget() that automatically creates the widget according to the kind of register we have. Moreover, the set and get attributes of the Registers handle themselves the gui updating such that timers are not needed anymore to maintain the gui in-sync with the redpitaya.

2. created a class BaseAttribute that is more general than Register: All the mechanisms that are expected to happen when the value of a register is changed (updating gui, saving the new value to a config file) is also sometimes needed for an attribute that is not directly a register (for instance scope.input1 was in fact a property that was modifying the register scope._ch1.input). For that reason, the set/get method hack described in 1 is implemented in an ancestor class of Register that is called BaseAttribute. Then the descriptor doing the trick scope.input1 --> scope._ch1.input is now a ScopeInputAttribute(SelectAttribute) that simply has custom methods get_value()/set_value(). However, the gui updating is done transparently as if it was a register and the create_widget method comes for free from the ancestor. These Attributes will be also useful to implement SoftwareModules that are not really mapping a module on the fpga, but which should be displayed as a widget in the gui, and should use parameter gui/saving mechanisms as if they were real modules (e.g.: curve fitting, or pyrpl_control)

   3. The list of Attributes that should be displayed in the gui are defined in a single place: ModuleClass.gui_attributes: a list containing the names of the attributes to display ModuleClass.setup_attributes: a list containing the names of the attributes to save in the config file

   4. In principle, if the gui display is as simple as exposing the gui_attributes in the gui, the default ModuleWidget class can handle it. However, sometimes more involved stuffs have to be done (for instance, adding a plot window for the scope...). For this reason, the particular class of widget that should be created when calling module.create_widget is configurable in ModuleClass.widget_class --> This solves a problem that I encountered a lot with the previous version: how do I easily subclass a module_widget to add my custom buttons without messing up with pyrpl's source code?

5. ModuleWidget instances are now accessed via module.widget (rather than at the same level before) which reduces the pollution of the redpitaya namespace.

6. Now (after oral discussion with Léo), I think we are OK to merge Pyrpl and Redpitaya into one class, such that Attributes can be saved in the config file that is currently only accessible to pyrpl level. To let the user decide whether he wants to load only the redpitaya level or the full pyrpl high-level functionality, the constructor of the class should take an argument that will let him decide which modules to load. The high-level pyrpl functionalities will simply be implemented in a particular SoftwareModule. Of course, if the user initializes the object with the name of a config-file, the list of modules to load by default will be read from the config-file.

7. In case the high-level pyrpl_control module is loaded, it would be nice to export the attributes of this module directly to the parent object (should we call it Redpitaya or Pyrpl?).

[lneuhaus]

1. Are you planning on adding documentation on the "Attribute logic"? Could you state the order of preferred reading of the code to understand what this means? For a person that has not read the code, the "active" and "default" state are not clear, among other things. I'll compile the Sphinx auto-documentation and try to understand your changes by reading that, just as a test, ok?

---> Let me know if you understood something to the attribute logic after reading the doc. BTW, is there a link to open the sphynx documentation in the web-browser ?

---> I agree that for non-gui users that do not have the widget's title indicating on what state they are, the autosave inside active state is very misleading. So probably, "states" will only be recallable, and autosave is only happening on the "working state". Let me know if you want me to add a flag "autosave" to allow the user to disable this functionality (on a per module basis?).

2. I agree with your remarks that doubt what can be gained by saving PID settings states. If I understand correctly, the idea is that with one command, you load a dictionary from a config file section with the name "pid__mylockedstate" that contains all PID settings, and directly overwrites all values? In principle not bad for certain settings. But I think this shows a little conflict that is arising (next point).

---> I agree with you: there is a good reason for PID settings to be completely delegated to the lockbox module. As you say, the calibration step and the expression of gains in terms of PI-corner frequency makes the lockbox the good place to store these parameters. Actually this use-case is exactly what "slave modules" were developed for.

---> On the other hand, in the case of scope and iir, the parameters that we define in the lockbox section is essentially a full copy of a scope state or iir state inside the lockbox module. I am not convinced the config file would loose in clarity if instead, we would only have a reference to a scope state and an iir state in whatever lockbox step needs one.

3. The idea of a config file was that the user could configure the lockbox behavior with a text editor. One of the clean-up actions I had planned was to reduce or clarify the config file structure. A clean file means:
   • no redundant information
   • logical and intuitive naming of sections and subsections
   • file as short as possible, in order to avoid confusion The danger of saving PID states is that this will create a mess in the config file, with redundancy between definitions and saved states. So either saved states, or configuration. With saved states the problem becomes that we need to compute some parameters from the configuration parameters and current calibration. One way to solve this would be to port the OutputSignal logic directly into the PID module. That is, the PID module is aware of the analog transfer function behind it and of the desired transfer function, and automatically chooses its gains accordingly (up to a global factor that is passed at the corresponding function call).

---> Would you find the approach described above clear enough (output parameters described in the lockbox sections and PID slaved during lockbox operation, iir and scopes loading states in each lockbox step, with only the state name in the lockbox section)? We then have to decide in which category we put the asgs and iqs (I guess an argument for slaving the iqs is that configuring a pdh in the iq might be non-trivial for beginners while the lockbox code provides a facility for that)?

---> I also had the crazy idea (to reduce the number of different logical layers) to use different "lockbox states" for the different steps. In fact, that is probably not such a great idea since only a small fraction of the lockbox attributes are redefined in each step and this would definitely not improve the readability to have 10 times the full lockbox configuration in the config file.

4. I agree with the CurveManager migration. Keep Django? What alternatives do we have, if we want to write and read with several processes into a database? Why discard django in the first place?

---> main problem: extra dependency, second is the memory management problem that we have experienced (in the end, the problem was that on a normal "website" operation, django refreshes its query cache memory after each html request, however since we never render html pages, this cache was exploding at some point). I tend to agree that since problem 2 is clearly identified and it should be easy to find a workaround and django is probably not the only non-standard dependency of pyrpl, we should go for it (also given the experience we have gained with it over the years). Alternatives would be lower level database apis (https://docs.python.org/2/library/sqlite3.html for exemple), but just thinking about writing raw SQL requests makes me feel tired.

---> Next question would be: do we want to stick with pandas? I must say I really hate it: it is the worst installation headaches we have had (with pytables). Moreover, the interface has been changed from one day to the next for such basic things as indexing (remember the iloc-related bugs?). I think it is probably not mature enough to be trusted, and the gain in terms of functionality is not tremendous (basically indexing with floats...)

[SamuelDeleglise]

You mean something more explicit than the docstring of BaseAttribute?

"""An attribute is a field that can be set or get by several means:
  - programmatically: module.attribute = value
  - graphically: attribute.create_widget(module) returns a widget to manipulate the value
  - via loading the value in a config file for permanence

The concrete derived class need to have certain attributes properly defined:
  - widget_class: the class of the widget to use for the gui (see attribute_widgets.py)
  - a function set_value(instance, value) that effectively sets the value (on redpitaya or elsewhere)
  - a function get_value(instance, owner) that reads the value from wherever it is stored internally
"""

I am definitely planning on having a step-by-step guide on how to create a new software_module and maybe another one on deriving a software module in the notebook. For instance, the example "derived class" could be the na with ringdown capabilities. For the new class, I am not sure, maybe something that would do some light-art with the leds of the redpitaya?

For reading of the code, I would go from simple to complex, that is start to read attributes.py, then modules.py, then pyrpl.py

[SamuelDeleglise]

I have documented more attributes.py and modules.py in a commit from this morning if you are reviewing the code right now...

[SamuelDeleglise]

If this is of any help, I managed to get the class diagrams for the main files of the code (I have unintentionally installed the professional version of pycharm on my laptop which can generate the diagrams within the 3 minutes of allowed usage). modules.pdf attributes.pdf

I am not sure the diagram is of any help for the attribute classes , because the blocks seem to be positioned in a weird way.

[SamuelDeleglise]

Sorry I hadn't seen your full post. I interleaved my replies inside your post

[lneuhaus]

I included the autodoc html build into the repository: pyrpl/doc/sphinx/build/html/index.html

To view the docs online, see issue #85

[lneuhaus]

So i read the attribute autodoc: doc/sphinx/build/html/pyrpl.html#pyrpl.attributes.FloatAttribute

Its quite satisfactorily described. I will add to the documentation as I read along..

[lneuhaus]

To summarize our discussion from yesterday:

• Pyrpl will be organized in a modular way as a collection of hardware_modules and software_modules
• the config file contains a section for each module class, containing the current state of the available module(s) and a subsection "states" which lists all saved states that can be recalled to overwrite the current module state
• Lockbox will also be a software module, with its state defined by: inputs, outputs, sequences, model quite similarly to previous definitions
• The messy config file will soon cease to be attractive for human intervention. The above changes will require a gui for the lockbox in the near future. The design of this gui is straightforward: select the model, provide the general parameters required by the model (i.e. wavelength, error_threshold), define a number of input signals (possibly from other hardware modules with associated saved states), define the output transfer functions (DC-gain, cutoffs), define locking sequences, add possibility to pass signals through other modules to create other signals (in principle already possible by defining other input signals based on previously defined ones fed through other modules. One issue will be: pdh signal generated with iq1 is to be passed through iir filter: IIR saved state has input iq1. When the lockbox is re-loaded, iq1 is used and PDH is generated with iq2. saved iir state must therefore somehow have a symbolic link to pdh signal..)
• CurveManager will be a software module that manages saved traces with a database (Django interface is still our favourite). We will remove the dependency on pandas and stick to pure np.arrays for the data. We should however allow the user to provide data in pandas.Series format for compatibility with earlier code, and automatically convert it to two np.arrays (this doesn't require dependency on pandas, only try..except blocks for a single data argument that tries to retrieve data.index and data.values)
• We should still think about reducing redundancy in the config file by defining default branches to keep it readable.

[lneuhaus]

I've made a few code comments in the pull request #83 . Let's continue the discussion there..

[SamuelDeleglise]

I agree with all your point, and already implemented 1 and 2.

Regarding point 4 (Lockbox gui): Just to make sure we are on the same line:

1. The input signal is a consequence of the model used (created using decorator in the model class), and allows to convert the measured value in V "at DC frequency" into physical units. It is implicitly assumed that the transfer function of the measurement is flat.
2. The output allows to convert the dac value in V into physical units "at DC-frequency". Moreover, a dropdown menu allows to specify what kind of analog transfer function lies behind the dac (low-pass filter with given cutoff, or even a database curve if the measurement is available).
3. Thanks to the previous fields, it is possible to get the "full physical _unit to physical_unit" open-loop transfer-function. Then instead of defining a "unity_gain_frequency" of the output, which becomes ill-defined when the transfer function is complicated, I suggest to define the gain of each output's loop with a 3 position switch (integrator only-->gain (s^-1), proportional only (gain), integral and prop-->PI corner frequency and P-gain) + an extra filtering stage. This gains are to be understood in terms of model-dependent input DC gain*output DC gain, and don't take into account the output's transfer function. To see what is the corresponding full-loop unity-gain frequency, there should be a plot window displaying in live the full open-loop transfer-function.
4. Then, each stage of the sequence only has to define the single input that is controlled and the list of outputs that participate with which "factor".

[lneuhaus]

1. At the moment, input signals can but must not be modeled by a corresponding function of the model. Only if an input signal is used as a lock input, a model function must be available. Therefore, the best way would be: Once the model is selected, prepare boxes for all input functions defined by the model, possibly leaving the input signal as not implemented in the specific lockbox configuration (e.g. if relection instead of transmission is used). However, we should allow to add custom-named additional input signals, which may be used to route signals through other modules (e.g. route the reflection signal through a PID module for lowpass filtering, only for diagnostic purposes, or add a second pdh signal for measurements). You're correct that the input signal is assumed flat over frequency at the moment. This has been a problem in an earlier version of pyrpl for a very narrow filtering cavity, where the signal had a detuning-dependent cutoff frequency within the locking frequency band. However, such specific things should be implemented in other models, e.g. fabry_perot_narrrow or sth alike.

2. Roughly correct: For the output, we should enable selection of a unit (we can define the allowed units in the model, which will currently include "m, Hz, degree, rad" depending on the model). Then, the DC-gain should be specified as a float in units_per_volt, possibly updated by a calibration. low-pass filters are easily specified by one or two cutoff frequencies. However, it is not clear to me how a transfer function should be automatically interpreted. But we can already implement this, and leave the interpretation of the curve for later.

3. If we do as you suggest, there is no need to specify the transfer function or cutoff frequencies of the output. However, in 90% of the cases one wants a pure integrator transfer function, and unskilled users may leave unity-gain-frequency at 1 kHz without problems for standard locks, without ever understanding what this means. So i suggest the following: Do not specify the analog transfer function of the output by default (contrary to the suggestion in 2). Only allow to define the PI gains, with P gain in units of 1 and i-gain in units of Hz, with a selector (or checkbox) to select whether the unity-gain of the integrator or the PI-crossover is specified. Add the plot window for the measured transfer functions. Add the filters. And add a button for aided loopshape design, where the previous fields are grayed out and instead, one can specify the analog transfer function by giving the analog cutoff frequencies and selecting a unity-gain frequency (a proportional gain does not make sense here because of stability reasons, although a LF-gain limit for the integrator might).

4. Unfortunately it is a little more complicated: first, each stage needs a "setpoint", i.e. a target detuning. Next, some stages need additional arguments, such as "offset" to ensure a drift of the integrator from the right direction. Then, we already have additional resonance search functions, which either sweep and stop at the resonance, or which are to engage a low-gain lock when the resonance is found. We also have to deal with the scope settings during the sequence. So I guess I would define the sequence as a list of functions to call with a dictionary of possible arguments, or even simpler: a list of dictionaries with a number of keywords with default values, such as {'call_function':'_lock', 'time': 0, 'factor':1, 'outputs': [], }. The default function to call is currently named "_lock". By the way, the sequences that are defined must not necessarily be associated to specific lockboxes, i.e. we might save sequences in a similar way as saved states of modules. And the gui should provide a field to copy the command that is needed to call a specific locking sequence from python code.

5. We should have a LED "islocked" and a button "auto_relock".

[SamuelDeleglise]

Signal slot implementation of gui:

Following our discussion from the other day, I think you are right that any gui updating should be done by signal-slot mechanisms. The main reason for that is that whenever pyrpl will be used inside a thread, we will run into problems because gui updating in Qt should always be done in the main thread (anything that is called by the event loop is in the main thread).

Right now, since for mostly everything, gui updating is confined into the base attribute logic, we only need to replace the descriptor.update_gui(module) function of BaseAttribute to some signal emission. Those signals should be connected at widget creation to the attribute_widget update function. This will also give the possibility to have several widgets representing the same module (don't ask me what it would be useful for...). Several questions remaining:

• For practical implementation, the logic behind Qt signal slot mechanicsm, is that they connect object1 to object2 (that should both be QObjects). Do we want the module to be a QObject that implement plenty of signals: input1_changed, duration_changed, average_changed, trigger_source_changed (one per relevant attribute).
• 2 other functionalities that are fully handled based on list of attributes at the module-level are
  • auto-saving in the config file (setup_attributes)
  • calling of callback (usually setup()) upon attribute change (callback_attributes),

It would probably make sense to also move these behind signal/slot mechanism.

As you saw the other day, there are some exceptions in the above-mentioned scheme. As far as I can tell, only the lockbox module should be concerned: Indeed, this module can also create or rename dynamically submodules (such as input, output, model, stage). That's why a few gui operations are not handled behind the scene by the BaseAttribute class --> I guess, the lockbox should simply emit custom signals such as input_created, output_created, stage_created, model_changed,..

Of course, having the modules also inherit from Qt makes Pyrpl much more Qt based than it used to be. What do you think about that ?

[lneuhaus]

this refactoring is done. Merge with master is imminent