Skip to content

Fix problem with repeated ensemble names in MISTIS#988

Merged
dwhswenson merged 1 commit into
openpathsampling:masterfrom
dwhswenson:fix-double-name-ensembles
Mar 2, 2021
Merged

Fix problem with repeated ensemble names in MISTIS#988
dwhswenson merged 1 commit into
openpathsampling:masterfrom
dwhswenson:fix-double-name-ensembles

Conversation

@dwhswenson

@dwhswenson dwhswenson commented Mar 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

In MISTIS, there's a distinction between the "input transitions," "sampling transitions" and "(physical) transitions", as described here:

openpathsampling/openpathsampling/high_level/network.py

Lines 742 to 750 in d32c198

The distinction between the three types of transitions in the object
are a bit subtle, but important. The `input_transitions` are, of
course, the transitions given in the input. These are A->B
transitions, but would allow any other state. The
`sampling_transitions` are what are used in sampling. These are
A->any transitions if strict sampling is off, or "A->B & not_others"
if strict sampling is on. Finally, the regular `transitions` are the
transitions that are used for analysis (use the sampling ensembles
for the interfaces, but also A->B).

However, OPS was auto-naming input transitions (and associated ensembles) with the same names as sampling transitions, potentially causing confusion when trying to load from storage. This PR fixes that by adding a name_suffix to the transitions, which defaults to an empty string. For input transitions, we use name_suffix=" (input)".

@dwhswenson dwhswenson added the bugfix PRs fixing bugs label Mar 1, 2021
@codecov

codecov Bot commented Mar 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #988 (972e533) into master (d32c198) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #988   +/-   ##
=======================================
  Coverage   80.28%   80.29%           
=======================================
  Files         136      136           
  Lines       14468    14471    +3     
=======================================
+ Hits        11616    11619    +3     
  Misses       2852     2852
Impacted Files Coverage Δ
openpathsampling/high_level/network.py 85.50% <ø> (ø)
openpathsampling/high_level/transition.py 41.19% <100.00%> (+0.66%) ⬆️

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 d32c198...972e533. Read the comment docs.

@dwhswenson

dwhswenson commented Mar 1, 2021

Copy link
Copy Markdown
Member Author

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

sroet
sroet approved these changes Mar 2, 2021
View reviewed changes

@sroet sroet left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can't wait to convert all these string summations to f-strings for OPS 2.0

@dwhswenson dwhswenson merged commit eda1160 into openpathsampling:master Mar 2, 2021
@dwhswenson dwhswenson deleted the fix-double-name-ensembles branch March 2, 2021 22:50
@dwhswenson dwhswenson mentioned this pull request Jul 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sroet sroet sroet approved these changes

Assignees

No one assigned

Labels

bugfix PRs fixing bugs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dwhswenson @sroet