theavey / paratemp Goto Github PK

The function should be able to take any dict arguments for either the SGE system or GROMACS. This should then be passed to some specific function. The specific keyword arguments should be put into this dict (if they're not already there), and passed to the sub-functions.

This is probably better than just taking a list or string arguments, though those could also be allowed with some type checking.

The py packages recommends it not be used in any new code, so it should be replaced with the use of the very similar pathlib/pathlib2 packages.

This should be fairly easy to do: just do regex to find numbers with arbitrary separators.

Could possibly just use numpy's or pandas' file importing functions.

I'm not sure what the equivalent argument is, but I would have to guess just box, as that is still a valid option.
I don't recall at this moment why box-forced was needed, but I think it had something to do with using shared axes.

Like calculate_distances, calculate_dihedrals shouldn't recalculate already defined things unless it is told to do so.

While fixing it, might as well read_data and save_data in the function as well.

At least the radius of gyration stuff doesn't really seem to make sense in a file named "energy_histo". There's also a function that uses that (make_rg_figures) that could be moved.

There may be others. I haven't checked at this point.

When I think of 'bins', I generally think of number of bins in x and y, but that turns out to be named n_bins, and bins has to do with z-binning. This should likely be changed, or at least the order of the arguments should be changed to make n_bins appear first.

This now claims python 3 compatibility, but that’s only true for mdanalysis 0.17+, so that version should be specified.

Universe.copy() tries to run the subclass init before copying the trajectory which results in an error.
Adding a custom invocation of copy could fix this and could actually copy over the new elements added in this package.

Some of this code works (only?) with Python 2, other parts with Python 3. That's not good.

For my code, it should be fairly simple to make it work across versions, but some of this is because of the compatibility of imported code.
This should be documented well so it's clear to users what works with which Python.

These defaults have worked fine for me for this one system, but there is no particularly good reason to have a default lowest temperature of 205 K, for example.

The default scaling exponent should also be removed. The number of replicas might be okay because 16 might be a somewhat common number of cores/threads per machine.

Script should go in scripts folder.

Notebook should go in examples folder.

travis currently uses just the local code, but it would be better (and good practice) to make the code install locally and then run the tests on that code.

Two (main) steps as I see it now:

• make the code installable
• change the imports in the test code to use the installed version and not the local module
• then, the top level should no longer need an __init__.py file.

It would be great if save_data() recognized at least that the columns were the same to save on read/write time.

Could try getting the column names with store.get_node("df").axis0.read().

At least for me, I generally want to run these anyway, so it would make sense to make them run by default to save as much time as possible. I don't imagine it would change the memory usage that much.
Plus, if it's saved, it's much easier to read it back in later if the RAM needs to be freed.

As implemented, this is really focused on parallel tempering-type replica exchange (where the temperatures differ between the replicas).

It would be good to ask someone who does non-temperature replica exchange to figure out how it would be best to do the indexing.
There might be some other parameter that might be natural to index by.

At the very least, possibly temps could be renamed to index or similar, and made optional.
Then, there could be a new argument index_type possibly that if it is 'temp', the replica's temperatures can be set by index.

No reason to have so much duplicate code

figures should likely be a dict, and things that produce figures should save figures to it with some default name that is settable (for fes_2d for example, it could be something like fes_2d_{x}_{y} if x and y have string representations that are nice?).

Doing this should clean up the name space a bit, especially for tab completion and such.

The FES methods should allow some sort of bin specification, even if it's just number of bins.

When distances are defined, it might be good to save those definitions to a separate dict.
Then, they could be accessed to calculate the same thing for a different trajectory, or remind the user of how they were defined in case the name is ambiguous.

In paratemp.para_temp_setup.extend_tprs, '-cpi' will be added even if it's already in the submission script, possibly crashing GROMACS.
This should be relatively easy to fix, just checking to see if it's already given in the line.

While you're at it, might be a good idea to do line.split('#') in case the end of the line is commented out.
I think that should work.

• add pytest-cov and coveralls requirements
• run coveralls after success
• add coveralls badge to readme

I am glad you find MDAnalysis and GromacsWrapper useful. I just wanted to point out that with the recent GW 0.7.0 release, Python 3 is also supported;

(If something is not working as expected, please raise an issue https://github.com/Becksteinlab/GromacsWrapper/issues – thanks!)

Putting the requirements into setup.py will allow them to be separated into those required for testing and those required for installing and using.

See this fork of Justin's and the setuptools docs.

A lot of work has been done extended MDAnalysis universes as replicas, but it would be great to have an object that would be a set of replicas for a single simulation.
This could make creating and iterating over all replicas must easier and cleaner.