Skip to content
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

Refactoring of recording_device #624

Closed
uahic opened this issue Jan 6, 2017 · 6 comments · Fixed by #1282
Closed

Refactoring of recording_device #624

uahic opened this issue Jan 6, 2017 · 6 comments · Fixed by #1282
Assignees
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: In progess DO NOT USE THIS LABEL

Comments

@uahic
Copy link
Contributor

uahic commented Jan 6, 2017

I would like to suggest some refactoring of the recording_device due to PR #577 and also because the class is pretty overloaded

Analysis:

All recorders do share the same language as they expect some Events as input in their handle routine. From this routine they usually call record_event of the recording_device class which in turn has been configured by the recorder. Considering the multimeter as an example, the first issue is the accumulator mode. This looks very specific to the multimeter and as the current implementation of the recording_device does exclude all recording to physical devices, many if-statements were introduced. Basically a large amount of the class is just concerned about checking for the different legal combinations. This also introduces a more or less tight coupling of the general recording_device to specific functionality of particular recorder classes.

My attempt to integrate MUSIC recorders into the recording_device in the current state would actually make the situation worse and bloat this class. The alternative would keep MUSIC devices either as a special case (current situation), so they don't use recording_device (thus, it is not possible to record to several channels at the same time and is more unintuitive) or pick one of various sub-classing possibilities.

Proposal:

There is only one recorder class for one purpose (no sub-classing for various channels) and no more music_<event/cont><in/out> special cases. This class aggregates a recording_device which in turn adds a new channel on request to an internal channel_list if the user requests 'to', possibly along with some set of parameters.
The record_event(...) method of recording_device then delegates the processing of the event in a chain-of-responsibility manner to all registered channels.

The channel base class defines a set of record_event functions with different Event-types to implement different handling. Possibly one could also use templates to solve this instead of overloading (haven't thought these details through yet). Status dictionaries are passed through until the recording_channel instances.

Two examples:

  1. Multimeter
    The user creates the multimeter and sets its accumulator mode to true, and also the 'to_file' flag. As channel logic and pre-processing of data is separated, the multimeter itself has to handle this special case by accumulating and repacking the result into new event instances before further delegate their processing.
    Multiple streaming to different channels that were not allowed before is now possible too.

  2. Spike detector
    The MUSIC as well as the standard spike_detector do not need to live in separate classes anymore. The special case that MUSIC does expect spike_times in seconds rather milliseconds is implemented in a MUSICEventOutChannel class.

Remark on MUSIC output devices:
Instead of labeling the synapses with channel id's the user is now forced to pass the info as an index array through the recorder.

@uahic
Copy link
Contributor Author

uahic commented Jan 7, 2017

The devices could also be provided by a manager which in turn allows flexibility for the user (such as integrating further networking libraries as extension modules, for example ROS for local purposes). I've started working on it but please let me know if you have any suggestions/critics

@heplesser
Copy link
Contributor

@uahic This sounds very interesting, although I am not entirely sure I understand all the details of your proposal yet. We may want to carefully analyse and evaluate the entire recording code before committing to a new design. Would you be willing to present you ideas in one of the next open developer VCs?

@heplesser heplesser added ZC: Kernel DO NOT USE THIS LABEL I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know ZP: In progess DO NOT USE THIS LABEL S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Jan 11, 2017
@uahic
Copy link
Contributor Author

uahic commented Jan 11, 2017

@heplesser Yes, the next VC would be fine!

Here is a more compressed version:

The (very simple) idea is basically to keep the recorder nodes and they are responsible for collecting Events and potentially to pre-process them (e.g. the multimeter's accumulator mode). Then they call record_event() of a recording_device_switch class which in turn only keeps a vector of actual recording devices and forwards events to them (= splitter). Each of these devices does exactly records events to one particular medium (FileDevice, StreamDevice, MusicDevice, RosDevice,...). To allow more flexibility, the recording_device_switch does not have a static list of devices but tries to get them via a DeviceManager. This is triggered when the user passes a 'record_to' list via SetStatus into the Recorder (Multimeter,...) which is propagated to the device_switch.

In this design there are (in the current state) some drawbacks:

  1. Each device could/should potentially handle all Event-subtypes which is a bit messy (so for each of them an overloaded record_event(...) function). I think this is a good point to discuss in detail

  2. The device_switch and devices use virtual inheritance which imposes more runtime-overhead. And this is called in a tight loop for each recorded event, maybe here is some space for optimization as well.

@apeyser
Copy link
Contributor

apeyser commented Feb 3, 2017

This conflicts with the nestio work. Before any refactoring is done, @wschenck and @jougs need to be pulled in.

@heplesser
Copy link
Contributor

This needs to be evaluated in context of NESTio work.

@heplesser heplesser added this to the NEST 3.0 milestone Apr 9, 2019
@heplesser heplesser removed this from the NEST 3.0 milestone Apr 9, 2019
@hakonsbm hakonsbm added this to the NEST 3.0 milestone Jan 10, 2020
@jougs
Copy link
Contributor

jougs commented Jan 10, 2020

This is partly addressed by the introduction of an infrastructure for recording backend in the nest-3 branch as of the merge of #1283. A similar infrastructure will soon be made available for handling input.

The part that is still missing then, is the porting of the MUSIC proxies to become backends in those infrastructures.

@jougs jougs removed this from the NEST 3.0 milestone Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: In progess DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants