moldyn / msmhelper Goto Github PK

Describe the bug
The state trajectory class StateTraj return properties as mutable references.

To Reproduce
Steps to reproduce the behavior, including a minimal working example.

import msmhelper as mh

# prepare data
traj = mh.StateTraj(np.arange(5))

traj.states  # array([0, 1, 2, 3, 4])
traj.states += 1  # AttributeError: can't set attribute

# BUT
states = traj.states
states += 2
traj.states  # array([2, 3, 4, 5, 6])

Expected behavior
Return only copies.

Runtime information:

• Version v1.1.0
• Source PyPI

This is part of the review process for openjournals/joss-reviews#5339

Thanks for bringing this wonderful package to the MSM world! I am wondering if you have considered conducting a benchmark comparison between msmhelper and Deeptime (https://deeptime-ml.github.io/latest/index.html). It would be fascinating to see how they compare. However, I understand if this is not a critical aspect of this package review.

This is part of the review process for openjournals/joss-reviews#5339

Hi again! My last comments are about examples and testing:

• The tutorials section of the documentation is very well done and easy to follow. My only suggestion is to write somewhere (in the README and perhaps also in the tutorials home page of the docs) that the jupyter notebooks are available in the docs/tutorials folder
• Very minor suggestion: It may be convenient to some users to have a local copy of the documentation, so why not adding a way of generating the docs html from the downloaded repo, which should amount to installing the requirements and running python3 -m mkdocs build or something similar.
• [ ] If I install all the testing requirements and run pytest, I get the following error at the end: FAIL Required test coverage of 90% not reached. Total coverage: 0.00%. Can you look into it?

This is part of the review process for openjournals/joss-reviews#5339

There's a few links broken in the github page

• https://github.com/moldyn/msmhelper#usage
  • linking issue with API
  • reference issue with following section
• https://github.com/moldyn/msmhelper#hummer-szabo-projection
  • linking issue with tutorial
• https://moldyn.github.io/msmhelper/benchmark/
  • Download doesn’t work

Updated May 3

• https://moldyn.github.io/msmhelper/tutorials/msm/
  • where λ i is the i -th eigenvalue.
    • of what?
• https://moldyn.github.io/msmhelper/tutorials/timescales/
  • raw latex in the webpage
    • e.g. t^\text{wt}
  • unfinished sentence
    • Here we see, that choosing a long lag time
• https://moldyn.github.io/msmhelper/tutorials/hummerszabo/
  • raw latex in the webpage
  • typo: neglegtable, evry

See PR conda-forge/staged-recipes#19893

Instead, normalize by row.

This is part of the review process for openjournals/joss-reviews#5339

• state indexing

  • I am a bit confused about how the state is indexed/reindexed. e.g. in estimate_markov_model. It is stated that "The states should start from zero" but I guess that's not necessary?

• plot_ck_test
  unfinished in the documentation

This is part of the review process for openjournals/joss-reviews#5339

Hi and thanks for the work you have put into this nice package. Here is a list of comments related to the paper:

• line 62: "Monte chain" -> "Markov chain" (I guess?)
• panels 1a, 1c and 1e look extremely similar to panels and figures of the Nagel et al (2023) paper. While this is not an issue per se, it may become a problem depending on the journal the foregoing paper will be published into. Please make sure that you'll have the rights to re-use the figure (or change the figure so that it won't clash with the journal license).
• About the figure: while I appreciate brevity, I think here additional details about the quantities shown in the figures would help an interested (but maybe not too knowledgeable, or just inexperienced) reader

Using mh.LumpedStateTraj.index_trajs the microstate index trajectories is returned instead of the macrostate index trajectories.