Skip to content

Conversation

@dwhswenson
Copy link
Member

Previously, storage.cvs contained all the storable functions. It should only include CVs (which are specifically storable functions that take a snapshot as input). This PR fixes that issue.

@dwhswenson dwhswenson added bugfix PRs fixing bugs experimental labels Mar 15, 2021
@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #995 (65bd120) into master (d303ff6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #995   +/-   ##
=======================================
  Coverage   80.58%   80.58%           
=======================================
  Files         138      138           
  Lines       14671    14671           
=======================================
  Hits        11823    11823           
  Misses       2848     2848           

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 d303ff6...65bd120. Read the comment docs.

@dwhswenson
Copy link
Member Author

This is ready for review and comment. I will leave it open for at least 24 hours, merging no earlier than Tue 16 Mar 20:00 GMT (21:00 local).

@sroet
Copy link
Member

sroet commented Mar 16, 2021

This LGTM. As you have labeled this as bugfix, what was the bug you encountered, anything you want to make sure doesn't regress? Or is it more a quality-of-life (and performance?) improvement for users to have an easier way loading cvs

@dwhswenson
Copy link
Member Author

It's not really something that I see as likely to have a regression; it's just that I chose the wrong class to use for the 'pseudotable'. The main usability improvement shows up in things like the CLI. For example, the contents command lists all the named objects in a file, organized by type of object (i.e., [cv.name for cv in storage.cvs if cv.is_named]). The idea is that the user can see what names are valid as command line options. Under CVs, SimStore storages were showing the generalized CVs that are automatically generated for TIS by the InterfaceSet (which enable disk-caching of maximum value of order parameter). But using those functions (which take trajectories as input) in place of a regular CV (snapshot input) would obviously cause errors.

Plus, under SimStore, there's a very clear distinction between StorableFunction (a generic SimStore object) and CollectiveVariable (an OPS-specific subclass of StorableFunction). If the storage attribute is called cvs, it should only return CollectiveVariables.

@dwhswenson dwhswenson merged commit 5ca0e40 into openpathsampling:master Mar 19, 2021
@dwhswenson dwhswenson deleted the simstore-cvs-only branch March 19, 2021 18:25
@dwhswenson dwhswenson mentioned this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix PRs fixing bugs experimental

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants