Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Geometry Refactor PR 1 #131

Merged
merged 31 commits into from
Sep 28, 2023
Merged

Geometry Refactor PR 1 #131

merged 31 commits into from
Sep 28, 2023

Conversation

shaoqx
Copy link
Collaborator

@shaoqx shaoqx commented Sep 27, 2023

This is the first PR for refactoring the /geometry module. It contains:

  • ARMer: migrate job manager from old EnzyHTP and resolved the compatibility issue
  • md_simulation(): finished the top-level-API code for running MD in general
  • enzyhtp_info: added EnzyHTPVersion for versioning

It also resolved issue #130

@shaoqx
Bugfix: fix position of the dict
e83014e
@shaoqx
Update: fix format
294d102
@shaoqx
Update: moved ARMer over from old EnzyHTP
f866a90
@shaoqx
Update: drafted the top-level design of sampling
faef513
@shaoqx
Update: create file structure for geometry
18078d7
@shaoqx
Update: finished the scaffold of md_simulation()
313e0bf
@shaoqx
Update: add more notes to MD software
9f5226c
@shaoqx
Update: reformat and fix path
91f9ea7
@shaoqx
Update: fix format
b111669
@shaoqx
Update: add enzyhtp_info and versioning functions
b156931 
and test
@shaoqx
Update: add watermark config
c467884
@shaoqx
Update: fit naming convention
5efd64f
@shaoqx
Update: halfway in fixing ARMer
8859bb0
@shaoqx
add note
c841911
@shaoqx
format
4bf5fe8
@shaoqx
add note
ce78fc4
@shaoqx
Update: add the ARMer paper
9c97e1c
@shaoqx
Update: fixed messed up importing in clusters
51c8cd9
@shaoqx
Fix: fix test answer
901d543
@shaoqx
Update: qouta style change
9643a05
@shaoqx
Update:fixed compatibility of ARMer in new EnzyHTP
fad8d3b
@shaoqx
format
8a64b0f
@shaoqx
Update: update amber version
3553241
@shaoqx
Update: set up test for amber interface dev
97597ff
@shaoqx
Update: match the ABC from future PR
b8ee640
@shaoqx
_
7fa1cba
@shaoqx
Update: loose end commit before PR
499e2c2
@shaoqx
Update: changed the design of connect and add test
84430dc
@shaoqx shaoqx linked an issue Sep 27, 2023 that may be closed by this pull request
Add bond order support in Atom().connect #130
Open
chrisjurich
chrisjurich requested changes Sep 28, 2023
View reviewed changes
Copy link
Collaborator

@chrisjurich chrisjurich left a comment

Choose a reason for hiding this comment

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

Very solid overall. As discussed, the long term plan is to make the ARMer usage more modular. However, that can be ignored for now. To approve this PR, I would like to see that the ARMerConfig() is altered to inherit from BaseConfig and the corresponding __getItem__ and __setitem__ methods.

enzy_htp/_config/armer_config.py Show resolved Hide resolved
enzy_htp/_config/system_config.py Show resolved Hide resolved
enzy_htp/core/clusters/accre.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should not go here. Every time that a new cluster is added, the package has to be edited/re-installed. This should probably be loaded from somewhere else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see what you mean by re-installed. Yeah this is a solid point. Thanks for bringing it! I added it to the issue. #132
However, adding new clusters won't be a frequent thing and are normally addressed during the installation process. I plan to have the open-source community supporting their own HPC and merge to the codebase. I will think about these different designs.

enzy_htp/core/clusters/expanse.py Show resolved Hide resolved
@chrisjurich chrisjurich merged commit 04d84da into develop_refactor Sep 28, 2023
@shaoqx shaoqx deleted the shaoqz/refactor_geometry branch September 28, 2023 03:21
@chrisjurich
Copy link
Collaborator

Looks good! Thanks for the changes!

@shaoqx shaoqx mentioned this pull request Sep 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisjurich chrisjurich chrisjurich approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add bond order support in Atom().connect
2 participants
@shaoqx @chrisjurich