Background

"Snapshot features" are a really powerful tool in OPS that make it easy to create a snapshot class based on the fields that should be associated with that snapshot. Those fields vary from engine to engine.

Unfortunately, the function that handles this, theattach_features decorator, is written in a way that makes it extremely hard to maintain or test. It is a single function taking over 500 lines in the file, which has a CodeClimate cognitive complexity of 203. (For context, CodeClimate recommends keeping that number under 5, and I find that anything over 10 or 15 can be a headache to deal with.)

Additionally, the documentation for snapshot features is not very good. The best docs are in the docstring for the_decorator method nested insideattach_features, which can't be viewed unless you actually read the fileopenpathsampling/engines/features/base.py. This is not easy to find, and even that docstring doesn't quite fully correspond to what the code does.

For these reasons, I want to reimplement theattach_features decorator, and simplify some the process of creating snapshot features.

A big part of the functionality ofattach_features is that it actually writes the code for several methods on the snapshot, including:copy,copy_to,create_reversed,create_empty,__init__, andinit_empty. The reason to writecode here, instead of just using the information in the__features__ object, is to get the correct signature and docstring for introspection. For example, we can do:

In [1]:importopenpathsamplingaspathsIn [2]:paths.engines.toy.Snapshot?Initsignature:paths.engines.toy.Snapshot(velocities=None,coordinates=None,engine=None)Docstring:Simulationsnapshot.OnlyreferencestocoordinatesandvelocitiesAttributes----------velocities :numpy.ndarray,shape=(atoms,3),dtype=numpy.float32atomicvelocitiescoordinates :numpy.ndarray,shape=(atoms,3),dtype=numpy.float32atomiccoordinatesengine : :class:`openpathsampling.DynamicsEngine`referencetotheengineusedtogeneratethesnapshot

where the signature and docstrings are determined by the features that are attached to the snapshot.

Changes in this PR

The goal here is to simplify both the process of creating new snapshot features and the code for attaching snapshot features, as well as to clean up some of the code that was autogenerated.

NewFeatureCollection class

The current implementation uses aFeatureTuple namedtuple which is populated by theattach_features decorator. This has been replaced with aFeatureCollection class. In general the approach is to create aFeatureCollection for each feature module using itsfrom_module method, and then to combine all theFeatureCollections into oneFeatureCollection bysumming them.

Simplerattach_features decorator

This replaces the oldattach_features decorator with a new one, which has considerably simpler code structure. First, all the features are gathered into a singleFeatureCollection, then the input class is decorated by adding the necessary functions, etc.

Separate code writing

Rather than including all the code for code writing/compiling in theattach_features method, this is put in a separate module, which should improve testability.

API break: Removingcopy,copy_to

Snapshots are immutable data-containing objects. I do not think there is a need for thecopy orcopy_to methods. There is, however, acopy_with_replacement method -- if you give it no arguments, it will create an exact copy of your snapshot with a different UUID. It also provides the ability to create modified version of snapshots.

API break: Removinginit_empty, fixingcreate_empty

I don't see a purpose forinit_empty that isn't already handled bycreate_empty (again, with the understanding that these are immutable objects).create_empty might be used as a template or a placeholder, so I'm keeping it around, but the code it currently generates is wrong: it should be aclassmethod, but it isn't marked as one, and it usesself in the signature butcls in the code.

Moving more functionality into superclass methods (newSnapshot base class)

Rather than writing out self-contained methods for each class, the new code keeps the core logic in a newSnapshot superclass, and simply passes the parameters on when needed. The key observation here is that we need dynamically-generated code for the purpose of signatures and docstrings, but the logic can be in general functions. This should be better for testing and for debugging.

NewParameter class to define field for a snapshot

In the old approach, you needed to go through the following steps to define a snapshot field (e.g.,coordinates orvelocities)

1. Add the name to thevariables list in the feature module
2. Decide how to change that feature on time reversal: if it was abool that flipped, add it to the list namedflip in the module. If it was a number that changed sign, add it to the list namedminus in the feature.
3. If it is a numpy array, add it to the list namednumpy in the feature.
4. If it will be loaded by a lazy proxy, add it to the list namedlazy in the feature.
5. Add a docstring to the module file, covering all thevariables included.

In the new approach, we have aParameter class that will contain the relevant information. With the old way, you may have needed to consider several top-level names for a given functionality (does this parameter go in theflip list for time reversal? in theminus list? neither? certainly not both, that would be nonsense -- but you could write code that tried that!) The new approach makes a class that describes each parameter, and the specific types of functionality (what to do on time reversal; how to copy; whether to lazy load) are contained in that object.

I think this is an easier way to think about these things, and it also has the advantage that instead of predefining certain behaviors (e.g., for copying), developers are welcome to create their own.

Time-reversal is treated a little differently than copying. Copying is an issue in computer programming, and is broadly relevant. Time-reversal is specific to MD. It's entirely possible that other such specific problems will arise in some subfield, so I tried to design a generic solution for them. Therefore,'time_reverse' is an entry in adict calledoperations. Theoperations can be extended with other domain-specific needs as they are found.

Decorator to declare instance methods to attach

If you want to attach functions that would become instance methods of your snapshot class, the old way was to define the function in the feature module and include its name in the list namedfunctions. The new way is to decorate the function with the@function_feature decorator.

(Temporary?) Code to generate newParameters from old style feature modules

I think the newParameter approach is more intuitive, but for now I've implemented methods inFeatureCollection that actually convert the old version to the new version. TODO: details to come.

Unchanged:@property

Just as before, and@property in a feature module becomes a property of the snapshot class.