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.

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

Add tests #4

Merged
merged 41 commits into from
Apr 23, 2021
Merged

Add tests #4

merged 41 commits into from
Apr 23, 2021

Conversation

jennyfothergill
Copy link
Member

@jennyfothergill jennyfothergill commented Apr 22, 2021

fixes #1 #2

  • big restructure--lots of deleting/moving functions around. now there are three main files (utils, cg_compound, and backmap)
  • I deleted any functions that weren't being used by my super simple example. If we need them later we can always go back. It's nice to reduce dependencies for now.
  • add unit tests
  • changed how the CG_Compound class works a little
  • Add a very simple Bead class

before merging

  • add pre-commit

delete wrap/unwrap use mdanalysis instead
move amber dict
conversion is more general
delete unused functions
@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #4 (8f076b5) into master (cd33454) will increase coverage by 84.98%.
The diff coverage is 97.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #4       +/-   ##
===========================================
+ Coverage   12.62%   97.61%   +84.98%     
===========================================
  Files           3        4        +1     
  Lines         578      168      -410     
===========================================
+ Hits           73      164       +91     
+ Misses        505        4      -501     
Impacted Files Coverage Δ
grits/finegrain.py 95.16% <95.16%> (ø)
grits/coarsegrain.py 98.48% <98.48%> (ø)
grits/__init__.py 100.00% <100.00%> (ø)
grits/utils.py 100.00% <100.00%> (+85.42%) ⬆️

@chrisjonesBSU
Copy link
Member

chrisjonesBSU commented Apr 23, 2021

Looks good to me. There are a couple of things I have questions about, or think might warrant a change.

  1. Both backmap and CG_compound use some kind of mapping scheme, but it's formatted differently for each module. backmap takes a dictionary of {bead: smiles} but CG_compound takes a list of [bead, SMARTS]. I think it would be great if the same format was used in both. In this case, I think a dictionary would be best.

  2. This is minor, and just a preference, but I think it would be good for the syntax to be more consistent between the two modules. For example, maybe they could be backmap.py and coarsegrain.py or finegrain.py and coarsegrain.py.

@jennyfothergill
Copy link
Member Author

jennyfothergill commented Apr 23, 2021

  1. Both backmap and CG_compound use some kind of mapping scheme, but it's formatted differently for each module. backmap takes a dictionary of {bead: smiles} but CG_compound takes a list of [bead, SMARTS]. I think it would be great if the same format was used in both. In this case, I think a dictionary would be best.

great point! the mapping operators are not what I want them to be yet. 🤔 Basically I want a CG_Compound to be able to be created using SMARTS or a mapping operator. The mapping operator would be solely based on particle indices and would be saved as an attribute to the CG_Compound when the compound is created from SMARTS. Allowing a CG_Compound to be created from a mapping operator would be helpful for using it on simulation outputs that are not in optimal conformations. e.g., We can get the mapping from the initial frame (ideal positions--recognizable to smarts matching) using smarts and use the mapping on the last frame (maybe weird conformation). Another update I want to make is that we keep track of the anchors and bonds in the CG_Compound as they’re created so that no mapping is necessary to the backmap function. For instance, the only arg to backmap would be the CG compound!

  1. This is minor, and just a preference, but I think it would be good for the syntax to be more consistent between the two modules. For example, maybe they could be backmap.py and coarsegrain.py or finegrain.py and coarsegrain.py.

I agree and I will make that change. I think finegrain and coarsegrain is maybe the most understandable.

@jennyfothergill jennyfothergill merged commit d3025e6 into cmelab:master Apr 23, 2021
@jennyfothergill jennyfothergill deleted the add/tests branch April 23, 2021 18:51
@jennyfothergill jennyfothergill mentioned this pull request Apr 23, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI/tests
2 participants