Feature/286 sysmodel logger

[juan-g-bonilla]

• Tickets addressed: Closes Bring parity between message recorders and variable loggers #286
• Review: By commit
• Merge strategy: Merge (no squash)

Description

Variable logging was implemented through a SysModel, which is added to a task similarly to message recorders. For usage, see documentation bskPrinciples 6.

The new SysModel logger take arbitrary lambdas as callback functions for logging, which are given in a dictionary. However, for recording simple module attributes, the utility function logger has been added to every module, which takes as input a string or list of strings and produces a logger that will record the given attributes:

moduleLogger = module.logger(["dummy", "dumVector"], macros.sec2nano(1.))
scSim.AddModelToTask("dynamicsTask", moduleLogger)

is equivalent to:

moduleLogger = pythonVariableLogger.PythonVariableLogger(
        {
            "dummy": lambda _: module.dummy,
            "dumVector": lambda _: module.dumVector,
        }, 
        macros.sec2nano(1.)
    )
scSim.AddModelToTask("dynamicsTask", moduleLogger)

The logger method is added to SysModel through a new swig file that all module swig files now include. An error is raised for scripts that try to access logger for modules that do not use the swig file (i.e. they use %include sys_model.h instead of %include sys_model.i).

The following methods are now deprecated:

• AddVariableForLogging
• GetLogVariableData
• RecordLogVars

However, they still work as expected. This was accomplished by having these functions call the new logging functionality internally, which means that the old logging has been completely removed from the code.

Verification

All scenarios and tests that used the old syntax have been updated to the new syntax, and all are passing. On a personal note, updating these has been a pain as it is difficult to script the conversion reliably.

Documentation

bskPrinciples-6 was updated to reflect the new way of logging variables. Note that the associated video is now outdated. Scenarios have also been updated to use the new syntax.

[juan-g-bonilla]

Some test errors are due to #288. Therefore, this PR is dependant on a fix to #288.

[juan-g-bonilla]

Added new C++ based loggers (see original post), which significantly speed up logging.

[juan-g-bonilla]

It was discussed that it is best to separate C++ logging from this new logging system, pending further discussion on whether we actually want to encourage 'variable logging' as a form of simulation poking.

[juan-g-bonilla]

Moreover, the possible merge of #317 might simplify the implementation of this PR (by leveraging the unified access to variables from the SysModel class for both C modules and C++ modules). Therefore, if #317 is expected to be approved withing the reasonable future, it might make sense to pause this PR until then. Thoughts?

[schaubh]

Sure, this PR can wait for #317 to be approved if it will yield a nicer implementation. It will be nice to have this, but there is no immediate urgency.

[schaubh]

@juan-g-bonilla , Looking over your PR for this branch. Nice work overall, I'm not seeing any major issues. Some questions:

• module swig files must be updated to include sys_model.i instead of sys_model.h, correct? This is regardless if the BSK python scrips uses module variable logging or not. If yes, we should add a warning to the Release notes that this version requires this small change. I reverse that swig file change for one module, compiled, and it still seemed to work. TO me at least this change in the swig file include wasn't required?
• The old data logging method is still supported as a deprecated functionality. Can we keep the old document on logging a variable in a new RST file as you do with Deprecated: Using old-style C modules? The old method is still supported for 1 year as a depreciated functionality, so I feel we should keep some documentation on the old functionality around.

[joaogvcarneiro]

Great change, makes it much easier. I added just a couple of comments, everything else looks great. Thanks, Juan!

[juan-g-bonilla]

@juan-g-bonilla , Looking over your PR for this branch. Nice work overall, I'm not seeing any major issues. Some questions:

* module swig files must be updated to include `sys_model.i` instead of `sys_model.h`, correct?  This is regardless if the BSK python scrips uses module variable logging or not.  If yes, we should add a warning to the Release notes that this version requires this small change.  I reverse that swig file change for one module, compiled, and it still seemed to work.  TO me at least this change in the swig file include wasn't required?

* The old data logging method is still supported as a deprecated functionality.  Can we keep the old document on logging a variable in a new RST file as you do with `Deprecated: Using old-style C modules`?  The old method is still supported for 1 year as a depreciated functionality, so I feel we should keep some documentation on the old functionality around.

Thanks for the review!

• Using sys_model.i instead of sys_model.h only makes the logger function available. Not using it will not break the module, but you cannot use logger. However, because we don't know when users might need logger and to prevent inconsistency, I would recommend we just tell users to update to .i (the release note is a good idea). Moreover, if we ever need to further customize the python wrapper of sys model, we will have all models already importing .i.
• I will bring back the old logging as a deprecated logging page!

Finally, I forgot to mention, there is a video tutorial that is now outdated (sorry!)

[schaubh]

Ok, let's add a note in the release notes that module *.i files must be updated to take advantage of the new logging system. Yes, I'll have to create a new video ;-). I'll work on that video after you have made these final tweaks.

[juan-g-bonilla]

@schaubh I implemented the small fixes, just waiting on the video for merging!

[schaubh]

Ok, thanks @juan-g-bonilla , thanks for the update. I'll try to do the new video for the RST file ASAP.

[schaubh]

Thanks for this PR. This is a nice change. I chose not to redo the video as the variable logging is now so simple, and similar to how the recorder() method works, I didn't see a need for a video.