force-h2020 / force-bdss-plugin-gromacs

Gromacs BDSS data sources, as well as a stand-alone wrappers around Gromacs tools.
MIT License
0 stars 0 forks source link

Include overwrite data option in SimulationDataSource #16

Closed flongford closed 4 years ago

flongford commented 4 years ago

This PR introduces an option on the SimulationDataSourceModel class to overwrite any simulation data if it already exists on file. Consequently, a new check is performed before running the simulation to see whether

If A returns False, then a simulation is always performed.

If A returns True, then a simulation will only be performed when B returns True also.

Whilst implementing this check, we have refactored some of the required functionalities of GromacsSimulationBuilder into the new interface class ISimualtionBuilder. This can be considered a step towards obataining generic simulation classes for the BDSS, similar to #14.

Change log:

codecov-io commented 4 years ago

Codecov Report

Merging #16 into master will increase coverage by 0.13%. The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   94.55%   94.68%   +0.13%     
==========================================
  Files          34       35       +1     
  Lines         826      847      +21     
  Branches       98      101       +3     
==========================================
+ Hits          781      802      +21     
  Misses         31       31              
  Partials       14       14
Impacted Files Coverage Δ
force_gromacs/gromacs_plugin.py 100% <ø> (ø) :arrow_up:
force_gromacs/api.py 0% <0%> (ø) :arrow_up:
...romacs/data_sources/simulation/simulation_model.py 96% <100%> (+0.16%) :arrow_up:
...romacs/pipeline/base_gromacs_simulation_builder.py 96.15% <100%> (ø)
force_gromacs/pipeline/i_simulation_builder.py 100% <100%> (ø)
.../data_sources/simulation/simulation_data_source.py 96.87% <100%> (+1.04%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 20e4453...55b306c. Read the comment docs.

flongford commented 4 years ago

Happy to merge then, if you think we want to keep some methods as NotImplemented for now.

Sure - similar to some objects in the force-bdss, the intention is for these objects to act as base classes for developers to implement. Do you think therefore it would be clearer to rename the class containing these methods to BaseGromacsSimulationBuilder?

Corwinpro commented 4 years ago

Happy to merge then, if you think we want to keep some methods as NotImplemented for now.

Sure - similar to some objects in the force-bdss, the intention is for these objects to act as base classes for developers to implement. Do you think therefore it would be clearer to rename the class containing these methods to BaseGromacsSimulationBuilder?

Good question, I didn't think about it - I thought that the GromacsSimulationBuilder is a ready to use class. How different can the NotImplemented methods be?

flongford commented 4 years ago

Happy to merge then, if you think we want to keep some methods as NotImplemented for now.

Sure - similar to some objects in the force-bdss, the intention is for these objects to act as base classes for developers to implement. Do you think therefore it would be clearer to rename the class containing these methods to BaseGromacsSimulationBuilder?

Good question, I didn't think about it - I thought that the GromacsSimulationBuilder is a ready to use class. How different can the NotImplemented methods be?

They have a fixed input / output signature, but otherwise are pretty generic - we could probably think of a better way to handle this using defaults but that might be out of the scope of the current PR