[REVIEW]: UPOsHam: A Python package for computing unstable periodic orbits in two degrees of freedom Hamiltonian systems

whedon commented 5 years ago

Submitting author: @WyLyu (WenYang Lyu) Repository: https://github.com/WyLyu/UPOsHam Version: v0.1.0 Editor: @kthyng Reviewer: @dpsanders, @JoshKarpel Archive: 10.5281/zenodo.3606676

Status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/bc6d9d9b9254ef94543016a4c40fafd0"><img src="https://joss.theoj.org/papers/bc6d9d9b9254ef94543016a4c40fafd0/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/bc6d9d9b9254ef94543016a4c40fafd0/status.svg)](https://joss.theoj.org/papers/bc6d9d9b9254ef94543016a4c40fafd0)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@dpsanders & @JoshKarpel, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @kthyng know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @dpsanders

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@WyLyu) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @JoshKarpel

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@WyLyu) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @dpsanders, @JoshKarpel it looks like you're currently assigned to review this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

JoshKarpel commented 5 years ago

@kthyng I'm somewhat concerned regarding the requirement that the software "be designed for maintainable extension (not one-off modifications)" (from https://joss.theoj.org/about#submission_requirements)

This submission is explicitly a set of examples, designed to be read and then modified by other researchers for their particular problems. That sounds a lot like "one-off modifications", because there's not really anything here to "extend"... guidance?

kthyng commented 5 years ago

@JoshKarpel Thank you for your question and work on your review, I will consult with the editorial board and get back to you soon.

Shibabrat commented 5 years ago

@kthyng I'm willing to explain the design decision of this package because it is related to the theory and applications of this method. Should I create a separate issue or briefly describe it here? Not sure about the etiquette at this point of review period.

Thanks

kthyng commented 5 years ago

@Shibabrat Why don't you give a brief description of the design decision here just to help deciding how to move forward, for now. One design suggestion is for you to change this to a library so that a user doesn't have to copy and paste to use it.

Shibabrat commented 5 years ago

Ok.

We have seen that researchers working in Hamiltonian dynamics have their favorite method to compute unstable periodic orbits. These methods are popular in chemical reactions, celestial mechanics, and engineering dynamics and always end up in appendices of the papers. Some of these papers are also in our references.

However, for anyone new or outside of the aforementioned communities, it becomes a slow tedious process to go through the code (even if available) and to adapt it for a specific problem. We think an illustration style set of transparent scripts that has minimal dependencies for installation, is easy to change and modify can help with learning the underlying methods.

Next, examples in the package are also in the form that is being used for ongoing work in our group on chemical reaction dynamics and hence gives a direct way to reproduce the results.

kthyng commented 5 years ago

@Shibabrat Your argument that this software is useful because it demonstrates going through some physics problems is convincing. However, it would be much better if your software was a proper python package that can be used in a typical manner in order to meet your goals. Specifically, can you change your setup so that another researcher using it can avoid copying and pasting bits of code into a new project, and instead import the full package and use reusable functions?

@JoshKarpel We think that the submission can be substantially improved, as per my first comment here, though your concern with the one-off modifications is ok — the point of the requirement is that the software have reuse capacity, which this appears to have. Still, changing this to a package will also help to address your concern.

JoshKarpel commented 5 years ago

@kthyng Sounds good to me. I think if it is not possible to make it more package-y, it needs to be very, very clear how and where to copy-paste-modify. But I would prefer a more package-y solution.

@Shibabrat @WyLyu some semi-concrete thoughts on how to move toward a package-based design...

So, my understanding is that there are three solution methods, which you use to solve three problems, leading to nine examples. Is it possible to decouple the code for each solution method from the problem it is solving, such that the same solution method code could be applied to any of the problems with only top-level configuration? As an incidental benefit, this would reduce the amount of code duplication in the project...

As a very concrete example, think about how the differential equation solvers in SciPy work ( https://docs.scipy.org/doc/scipy/reference/generated/scipy.integrate.solve_ivp.html#scipy.integrate.solve_ivp). There's a high-level solver which can apply different methods (set via the method argument) to whatever the user passes in. Many different differential equations in many different forms can be solved by the solver (although the user may need to select the correct method for their given problem, perhaps equivalent to selecting a solution method in your code?).

Shibabrat commented 4 years ago

@kthyng @JoshKarpel We have modified, and doing so to the rest of the package as I write this, the design in a way that minimizes the copy-pasting of the code segments for a new problem. However, we would like to emphasize why writing something like the solve_ivp type package is not the way to go.

The methods implemented in this package are meant for computing the existence of unstable periodic orbits for nonlinear and chaotic systems. While a numerical ordinary differential equation solver is a component of such methods, determining the existence of specific solutions, such as periodic orbits, requires additional information on the structure of the phase space. As opposed to linear ordinary differential equations, distinct nonlinear differential equations often have radically different behaviors, e.g. chaos, integrability, invariant phase space structures, etc. Consequently, existing general packages are inadequate for analyzing such diverse dynamics that can occur in systems that, on the surface, appear to have a similar structure.
The methods that we present get around some of these difficulties by focussing on specific features of the potential energy surface (PES) that may be common in seemingly diverse applications. In particular, the unstable periodic orbits that are computed using our methods are dependent on the specific features of the topography of the PES that can be checked in specific applications.

So to achieve a compromise between this view and a user-friendly design, we have combined the problems under each method with an option to choose. A new user can walk through the functions in a method and change the problem-specific expressions (that they need to derive) to get the results for the problem.

JoshKarpel commented 4 years ago

@Shibabrat I want to make sure I understand what you're saying at a high level: there's no way to decouple the application of the methods from the problems, because the details of the method need to take into account the specific nature of the problem?

I ask because, looking at the code (at my level of understanding, which I admit does not extend to understand the actual solution methods), I don't see that happening. The most common thing is functions like https://github.com/WyLyu/UPOsHam/blob/master/tp_UPOsHam2dof.py#L103 , which are just switch statements on defining the problem to be solved. In a solve_ivp-like package this function would be provided by the user and passed into the package. Other functions look like https://github.com/WyLyu/UPOsHam/blob/master/tp_UPOsHam2dof.py#L310, where there is a switch on the model followed by a piece of generic code that would work on any of the models. Then there are functions like https://github.com/WyLyu/UPOsHam/blob/master/tp_UPOsHam2dof.py#L335 that don't depend which model you're solving at all.

This lack of separation is what would make it difficult to reuse this code. In fact, all of these switch statements make it harder to understand which parts the user has to modify to solve their own problem, not easier. Requiring the user to pass in things like "a function that returns the Hamiltonian" or "a function that returns the potential energy" with specific signatures is much better than hard-coded switches buried inside functions.

Could you point to some specific places in the code where the solution method and the problem cannot be decoupled from each other? i.e., some special knowledge about the problem is used in the solution method in a way that cannot be generalized to other problems.

Shibabrat commented 4 years ago

@JoshKarpel Yes, you are correct. Some (I point to those below) details of the methods can not be decoupled from the specific features of the problem. In particular, one such feature is a detailed knowledge of the bottleneck region of a potential energy surface (PES). For a given molecular system, the PES is the output of a quantum chemistry calculation and the results are highly dependent on the molecule.

The first-order correction based on the monodromy matrix for the differential correction needs derivation based on how the bottleneck is oriented in the configuration space. The correction term is different in https://github.com/WyLyu/UPOsHam/blob/master/diffcorr_UPOsHam2dof.py#L744 from https://github.com/WyLyu/UPOsHam/blob/master/diffcorr_UPOsHam2dof.py#L759, and it can also be same as in https://github.com/WyLyu/UPOsHam/blob/master/diffcorr_UPOsHam2dof.py#L752. This correction is applied to the phase space coordinate that is most natural for the bottleneck of the PES. Fixing this coordinate also determines the coordinate that is used to check convergence (https://github.com/WyLyu/UPOsHam/blob/master/diffcorr_UPOsHam2dof.py#L651) and the one that is used to terminate the integration using event function (https://github.com/WyLyu/UPOsHam/blob/master/diffcorr_UPOsHam2dof.py#L303).

The turning point based on coordinate difference relies on the orientation of the PES to choose whether to use x or y or both coordinates to check which direction the trajectory is turning. In this case, the orientation of the bottleneck of the PES determines how the turning direction is checked (https://github.com/WyLyu/UPOsHam/blob/master/tpcd_UPOsHam2dof.py#L362).

The turning point method probably can be generalized but then one needs to manually choose two initial conditions near the final/true UPO. This is probably the one that comes closest to being written without problem-specific details as long as the user starts with two assumptions: first, the two starting initial conditions for the method should be close (whatever that means for the problem) to the final UPO and second the accuracy can't be improved beyond 10e-3 (which is fine for rough estimates or to restart the TPCD method but not for using the UPO in other quantitative calculations such as transition fraction or reaction flux)

Having said that, I do think some portions of the package can be separated as user-supplied functions with specific signatures, as per your suggestion.

JoshKarpel commented 4 years ago

@Shibabrat When I see a block like https://github.com/WyLyu/UPOsHam/blob/master/diffcorr_UPOsHam2dof.py#L738 , it seems clear to me that this calculation is part of specifying the problem to be solved, and should be passed into the solver by the user. In that case it looks like the input parameters are par, x0, x1, and phi_t1, and the function should return the corrected x0.

If that means you have to pass ten different functions (or parameters, or whatever) into the solver, so be it: in the current version the user would have to do that anyway, but by copy-paste-editing long functions instead of writing their own small, focused functions. I think that passing configuration to the solver, even if there is a lot of it, is a far friendlier than being asked to make specific edits in hundreds of lines of code.

The examples would then largely be the definitions of the functions and configuration parameters for the appropriate problems, which would be much easier for people to take and re-purpose for their own problems.

Shibabrat commented 4 years ago

We have updated the methods by separating the problem-specific functions. The demonstration script which loads the method as module is the starting point for a user to modify according to their problem of interest.

JoshKarpel commented 4 years ago

I poked around in the latest version, and I definitely like where this is heading! Now that I think we're on the right architectural path for a reusable package, I'll start making issues in the repository again to address specific issues.

kthyng commented 4 years ago

Hi @dpsanders Will you be able to work on your review soon? Changes are being made in the package currently, but it looks like they should be winding down soon.

dpsanders commented 4 years ago

@kthyng Sorry, I hope to be able to during the next few days.

dpsanders commented 4 years ago

I suggest that all future changes to the repo are done via Pull Requests, so that we can easily see what has been changed.

dpsanders commented 4 years ago

(Each Pull Request should ideally address a single, or a few related, points from the reviewers.)

dpsanders commented 4 years ago

The suggestions made by @JoshKarpel about the code structure are spot on. There is still a lot of duplication between files like run_diffcorr_POfam_coupled.py and run_turningpoint_POfam_coupled.py that start out by defining exactly the same functions (as far as I can tell).

This makes it difficult to know if any copy-paste errors have been introduced and difficult to identify the basic structure and key parts of the code.

All of this code duplication should be eliminated.

dpsanders commented 4 years ago

There are also still no automated tests.

JoshKarpel commented 4 years ago

@Shibabrat @WyLyu please make sure to reference the issues (and specific commits, where reasonable) from the pull requests so we know which is addressing what.

Shibabrat commented 4 years ago

We've mostly done the last few changes that include the filename, moving code to different directories, and structuring documentation using pull requests. But maybe we didn't tag or reference the issues so that's why it's not clear what we have been changing. Apologies for the confusion. We are cutting our teeth with the branching and pull requests on different but related issues.

In summary, we are working on the automated tests and removing the duplication in the scripts. These sets of changes will also address how to run the scripts without the figures interfering with the progress of the iteration.

JoshKarpel commented 4 years ago

@Shibabrat @WyLyu The PR churn is a little hard to follow. I'd appreciate it if you could batch your changes a little more, explain what's going on in each PR, and write comments in the PRs explaining which issues you're addressing with them. Not everything needs to be merged to master immediately; you can checkout a branch that someone else pushed to GitHub and work on it locally, then push more commits to it, without merging the PR.

Net result: please only merge a PR to master when you're confident in the changes, and have added a comment on the PR explaining them.

kthyng commented 4 years ago

@Shibabrat @WyLyu What is your status on comments from your reviewers?

Shibabrat commented 4 years ago

We have now started two PRs (https://github.com/WyLyu/UPOsHam/pull/34, https://github.com/WyLyu/UPOsHam/pull/35) that will resolve most of the issues currently pending. @JoshKarpel and @dpsanders can you please let us know if it makes sense what we have addressed? We can then go ahead and merge them or make appropriate changes.

kthyng commented 4 years ago

Just another quick ping to check in on thoughts from @JoshKarpel and @dpsanders!

JoshKarpel commented 4 years ago

@kthyng We're definitely making progress. The authors have made a lot of good changes, and I think this big PR finally gets us to a place where they can start satisfying the review requirements directly (things like setting up documentation and making it an installable package).

kthyng commented 4 years ago

Awesome!

labarba commented 4 years ago

👋 @dpsanders — Your last comment from Oct. 17 says:

There are also still no automated tests.

Do note that JOSS review criteria regarding tests is that "Authors are strongly encouraged to include an automated test suite…" but "Documented manual steps that can be followed to objectively check the expected functionality of the software" are OK.

Given this, and that this review has been ongoing for three months already, and the authors have made many improvements, are you ready to submit a recommendation?

dpsanders commented 4 years ago

There has definitely been a lot of progress, but as @JoshKarpel comments above, there are still some fundamental issues that the authors have not finished addressing, such as docs.

Shibabrat commented 4 years ago

thank you @labarba

@dpsanders and @JoshKarpel the package documentation is under progress (https://uposham.readthedocs.io/en/latest/#) since the recent PR contains the updated docstrings and changes to calling the scripts from the command line.

Test (comparing the analytical and numerical solution) in the form of a notebook will also reflect these changes.

Shibabrat commented 4 years ago

@kthyng @dpsanders and @JoshKarpel we have made some recent updates to the documentation and tests (this is available in the test directory as tests.ipynb) that are compatible with the module structure of the methods.

The correctness test will soon be restructured as an assert test along with the visual comparison (at the moment).

API documentation will feature example code snippets and reference to a script that solves the provided example systems using a method.

We would appreciate some feedback on these new developments.

JoshKarpel commented 4 years ago

I submitted some new issues with high-level feedback on the docs. Major progress!

The tests are moving in the right direction; very happy to hear that you'll be able to do an actual assertion. There should be some information in the README about how to run the tests and how to do any manual verification, but that can wait until the structure settles down.

kthyng commented 4 years ago

@Shibabrat Looks like your documentation should be your main focus now.

Shibabrat commented 4 years ago

@dpsanders @JoshKarpel Can you take a look at the following PRs?

We've updated the documentation and started a PR: https://github.com/WyLyu/UPOsHam/pull/50. This also includes a plain text explanation of how the methods and the systems fit together.

The test branch and it's PR: https://github.com/WyLyu/UPOsHam/pull/49. This includes the unit test for all the methods.

Both these should address most of the pending issues and requirements upon merging. We look forward to your feedback.

Shibabrat commented 4 years ago

@kthyng We have now updated the documentation (https://uposham.readthedocs.io/en/latest/) and the new tests have also passed (https://github.com/WyLyu/UPOsHam/pull/49). How do we proceed now?

ooo[bot] commented 4 years ago

:wave: Hey @Shibabrat...

Letting you know, @kthyng is currently OOO until Tuesday, January 7th 2020. :heart:

kthyng commented 4 years ago

@Shibabrat Looks like #49 is good to go from the reviewers and maybe we can hear on #50 from @dpsanders.

I see there are a few more small tasks to clean up. Otherwise, @dpsanders and @JoshKarpel, could you give an overview of what's left to do at this point? I see unchecked boxes but perhaps some can be checked off now? Thanks very much.

JoshKarpel commented 4 years ago

The last major fix that I see is https://github.com/WyLyu/UPOsHam/issues/55. I think I'll be ready to approve once the fix for that is merged.

Shibabrat commented 4 years ago

@kthyng @JoshKarpel @dpsanders the pending PRs https://github.com/WyLyu/UPOsHam/pull/57, https://github.com/WyLyu/UPOsHam/pull/58, and https://github.com/WyLyu/UPOsHam/pull/59 are now ready for review. The major update is the package is now installable as a module and the scripts can load the system-specific functions and method-specific functions using, for example,

import uposham.differential_correction as diffcorr
import uposham.deleonberne_hamiltonian as deleonberne

As these PRs get approved, I'll merge them and run tests again.

ooo[bot] commented 4 years ago

:wave: Hey @Shibabrat...

Letting you know, @kthyng is currently OOO until Tuesday, January 7th 2020. :heart:

Shibabrat commented 4 years ago

@kthyng @dpsanders @JoshKarpel we are ready for the next step of the review. The tests have also passed after the recent merge. How do we proceed now?

ooo[bot] commented 4 years ago

:wave: Hey @Shibabrat...

Letting you know, @kthyng is currently OOO until Tuesday, January 7th 2020. :heart:

JoshKarpel commented 4 years ago

I'm happy with the changes, have checked off all my boxes, and recommend acceptance.

dpsanders commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

Shibabrat commented 4 years ago

@dpsanders I'll go through this version to check for typos and update the pdf. Do you want us to address any of the 3 remaining things in your checklist?

@JoshKarpel thanks for the approval.

openjournals / joss-reviews