New storage system with auto-magical variable handling

[Lnaden]

This is the new storage system module which handles hard drive IO for arbitrary data sets with minimal user input. Currently handles NetCDF storage, but can be extended to add new storage modules with from the abstract class.

Handles the following types:

• String
• Int
• Float
• List (any size)
• Tuple (any size)
• Quantity (of any of the previous types)
• Dict (entries can be any of the previous types with string keywords)
• numpy.ndarray (and Quantity(ndarray))

The StorageInterface module allows auto-magical data IO such that you define your directories and variables on-the-fly with commands such as

StorgeInterfaceInstance.my_directory.my_variable.write(data)

where a folder called my_directory and my_variable are created for you with data being automatically encoded to the storage medium and saved. Accessing it is done with

StorgeInterfaceInstance.my_directory.my_variable.read()

and the data at that same location is fetched and automatically decoded for you. No template nor pre-determining structure required.

@andrrizzi Main review points are the changes to the math_eval function, the main __init__, and the new quantity_from_string function. Functionality of storage code unchanged from the YANK PR, unless there are other issues you see with it.

---

This is a migration of the code from choderalab/yank#617 to make this module stand separate from YANK as others may want to use it.

Tagging those that have expressed interest in this @bas-rustenburg @gregoryross @ChayaSt, I would appreciate any feedback you have on the code.

[Lnaden]

One problem with this is that NetCDF4 on the main channel is only available on numpy 1.11 and not 1.12 like the one on conda-forge. So this package now sometimes causes numpy downgrades.

[andrrizzi]

Sweet! I dug deeper into the mechanisms and add a bunch of minor points. The only important comments are the order of functions.update in math_eval, and the class attributes that may get out-of-date. The rest is just feedback on code readability.

Feel free to merge as soon as you are satisfied.

[andrrizzi]

You could think of returning self._storage_system.file_name here and ditch self._file_name to avoid keeping the same information in two places.

[andrrizzi]

You could think of returning self._storage_interface.storage_system here and ditch self._storage_driver to remove redundancy.

[Lnaden]

I had originally created that variable as a protected name so you could not assign it as one of the variables on disk. You are right, I can just derive this and all it did was expose a hidden objects' property. I did a small refactor to remove this and just derive the object.

[andrrizzi]

Duplicate line.

[andrrizzi]

What policy do we have about versions? We haven't released 0.9.0 yet, and we don't store every single development version on conda (or do we?), so I think we could bump only after release. This way we won't have "missing" versions in the conda channel.

[andrrizzi]

Why splitting and joining? Could this work instead?

if self.predecessor is not None:
    return self.predecessor.path + ('/' + self.name)  # the parenthesis just makes it a little faster
else:
    return self.name

[andrrizzi]

Maybe calling these NCVariableCodec to adopt a consistent nomenclature? Or maybe you could rename the function set_codec to set_type_handler?

[andrrizzi]

I may be confused here, but could you rename parent_handler to netcdf_driver? And is there a use case in which we want to have a different storage_object than netcdf_driver.ncfile?

[Lnaden]

Renamed them to parent_driver to remove confusion.

Yes! The storage object can either be the top level ncfile OR a group/subgroup within that file.

[andrrizzi]

One way you could remove code redundancy here is to use something like:

# This works is for everything but strings.
def dummy_encoder(data):
    return data

# This works for ints and floats.
def scalar_encoder(casting_type):
    def _scalar_encoder(nc_variable):
        data = nc_variable[:]
        if data.shape == (1,):
            data = casting_type(data[0])
        else:
            data = data.astype(casting_type)
        return data
    return _scalar_encoder

[andrrizzi]

Did you want to do the opposite?

functions = {'step': ..., 'step_hm': ...}
if user_functions is not None:
    functions.update(user_functions)

or did you wanted the default functions to overwrite a custom step function?

[andrrizzi]

If it can simplify your life, you could also make use of inspect.getmembers(unit, predicate=inspect.isfunction).

[Lnaden]

Eh, this yields the same result and avoids another import. Its also not too hard to read, so I think I'll keep it.

[andrrizzi]

The new dictionary codec looks good! Added three more comments, but feel free to merge when ready.

[andrrizzi]

Maybe we should call these two just unit and value? This way we'll be able to use the same serialization in other formats without tying it to netcdf.

[Lnaden]

Are you suggesting generalizing the dict YAML Loaders/Dumper for future drivers?

[andrrizzi]

No no this is fine for now, but if in the future we want to do this somewhere else, we'll need a dict to str serialization that has not NC in the name and we'll have to duplicate the code.

[andrrizzi]

If you just return self.dtype instead of None, you can remove the hybrid property/getter _get_on_disk_dtype and you would still be able to overwrite the behavior.

[Lnaden]

Can't do that. The self.dtype property is how the IODriver identifies what codec to use based purely on the input data. It processes type(data) and reads the codec from the internal database. The problem with the dict type is that its a subset of NCScalar which used to pass self.dtype to the NetCDF variable creation routine, but dict is not a type NetCDF can handle, so I had to come up with a way to define a mappable type in the codec, without breaking the IODriver's auto-data-detection methods.

[andrrizzi]

Sorry, I think I wasn't clear. I'm just suggesting to substitute this:

    @property
    def _get_on_disk_dtype(self):
        """Function to process None for _on_disk_dtype"""
        if self._on_disk_dtype is None:
            return_type = self.dtype
        else:
            return_type = self._on_disk_dtype
        return return_type

    @property
    def _on_disk_dtype(self):
        """
        Allow overwriting the dtype for storage for extending this method to cast data as a different type on disk
        This is the property to overwrite the cast dtype
        """
        return None

with a single

    @property
    def _on_disk_dtype(self):
        """
        Allow overwriting the dtype for storage for extending this method to cast data as a different type on disk
        This is the property to overwrite the cast dtype
        """
        return self.dtype

i.e. get rid of the hybrid _get_on_disk_dtype property. Unless there's something I'm missing.

[Lnaden]

I see now, yes, that I can do

[andrrizzi]

Can you add a Quantity-wrapped numpy array as a test case here to make sure it works if it's not somewhere else?

[Lnaden]

Line 207, already does it!

[andrrizzi]

Doh! Didn't realize, sorry.

[andrrizzi]

Awesome! I love all the code simplification in the NCVariable classes.

[andrrizzi]

You may not need this line (the property doesn't seem to set a member).

[andrrizzi]

Here too (and in few other places).

[Lnaden]

I've refactored this a bit more to make _bind_read a non-abstract method, also moved parts of _bind_append and _bind_write functions to a common call.

[andrrizzi]

Looks great! I just realized I forgot to import the changes to the dictionary codecs from yank. I'll do it asap.

[jchodera]

Feel free to review and merge this without my involvement!

[Lnaden]

There are some bugs I have been working out and @andrrizzi got the review in, once I finish up the last few conflicts (and travis stops fighting me), I'll be able to merge this

[Lnaden]

So the windows builds now fail because tqdm and openmm conflict. There is no Windows Python 3.4 cut for tqdm, and I don't think there was one for OpenMM either, the windows tests have been using OpenMM 6.3 from what it looks like. This also strikes me as odd.

[jchodera]

This will be fixed as soon as we add conda-forge as a dependency.

[Lnaden]

I'm assuming that will also fix the Linux Python 3.4 build not finishing either, since I assume its tqdm missing again (not on the anaconda channel). A few points though:

1. The Python 3.4 tests are garbage since they dont build and instead pull packages from the net
2. Do we really need that progress bar on one test thus requiring a whole new package?
3. We should drop Python 3.4 support anyways once we get a stable OpenMM on all platforms for python 3.6

[jchodera]

Do we really need that progress bar on one test thus requiring a whole new package?

We can ditch that.

We should drop Python 3.4 support anyways once we get a stable OpenMM on all platforms for python 3.6

I can't stress this enough: We MUST do whatever conda-forge does regarding package support for now. Let's not drop support for anything until conda-forge does.

[Lnaden]

Okay, this is back into a state it can be merged in. I don't have it handle the compressed, fixed length dictionaries like we do in Yank's dicts when writing to NetCDF, but I don't think we quite need that yet. Did you want to take a look at this to make sure it does all you need to, @jchodera?

[jchodera]

Awesome! Thanks for adding docs!

Let's go ahead and merge this in and we can improve it from here.

[Lnaden]

Will do if this last test does not time out again!