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

print the hierarchy of a compound #1097

Merged
merged 38 commits into from
Mar 14, 2023

Conversation

chrisiacovella
Copy link
Contributor

@chrisiacovella chrisiacovella commented Mar 7, 2023

PR Summary:

This refers to issue #1096 . This adds in functionality to print out the hierarchy using treelib (https://treelib.readthedocs.io/en/latest/).

An example output of a system with two ethane molecules is shown below. The function will automatically group together compounds at the same level that are identical in name, number and name of children and number and name of particles.

Compound, 16 particles, 14 bonds, 2 children
└── [Ethane x 2], 8 particles, 7 bonds, 2 children
    └── [CH3 x 2], 4 particles, 3 bonds, 4 children
        ├── [C x 1], 1 particles, 4 bonds, 0 children
        └── [H x 3], 1 particles, 1 bonds, 0 children

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

mbuild/compound.py Fixed Show fixed Hide fixed
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10 🎉

Comparison is base (00d8c77) 89.29% compared to head (4a0434b) 89.39%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1097      +/-   ##
==========================================
+ Coverage   89.29%   89.39%   +0.10%     
==========================================
  Files          61       61              
  Lines        6173     6235      +62     
==========================================
+ Hits         5512     5574      +62     
  Misses        661      661              
Impacted Files Coverage Δ
mbuild/compound.py 96.95% <100.00%> (+0.18%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chrisiacovella
Copy link
Contributor Author

Linux tests are being reported as failing, but it appears it is actually just being canceled when testing the silica interface. Basically, same thing as discussed in #1090 . I'm going to go ahead and comment out those tests of the next commit.

mbuild/tests/test_compound.py Fixed Show fixed Hide fixed
mbuild/tests/test_compound.py Fixed Show fixed Hide fixed
Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

This looks great, some comment about the identifiers (to avoid false positive)

mbuild/compound.py Outdated Show resolved Hide resolved
mbuild/compound.py Outdated Show resolved Hide resolved
mbuild/compound.py Outdated Show resolved Hide resolved
mbuild/compound.py Outdated Show resolved Hide resolved
@daico007
Copy link
Member

daico007 commented Mar 9, 2023

Also, pre-commit is failing due to docstring style issues

check yaml...............................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
black....................................................................Passed
isort (python)...........................................................Passed
pydocstyle...............................................................Failed
- hook id: pydocstyle
- exit code: 1

mbuild/compound.py:283 in public method `print_hierarchy`:
        D205: 1 blank line required between summary line and description (found 0)
mbuild/compound.py:283 in public method `print_hierarchy`:
        D400: First line should end with a period (not 'e')
mbuild/compound.py:283 in public method `print_hierarchy`:
        D401: First line should be in imperative mood (perhaps 'Print', not 'Prints')
mbuild/compound.py:283 in public method `print_hierarchy`:
        D214: Section is over-indented ('Parameters')
mbuild/compound.py:283 in public method `print_hierarchy`:
        D215: Section underline is over-indented (in section 'Parameters')
mbuild/compound.py:283 in public method `print_hierarchy`:
        D214: Section is over-indented ('Returns')
mbuild/compound.py:283 in public method `print_hierarchy`:
        D409: Section underline should match the length of its name (Expected 7 dashes in section 'Returns', got 6)
mbuild/compound.py:283 in public method `print_hierarchy`:
        D215: Section underline is over-indented (in section 'Returns')
mbuild/compound.py:380 in private method `_get_hierarchy_nodup`:
        D205: 1 blank line required between summary line and description (found 0)

chrisiacovella and others added 10 commits March 9, 2023 16:30
marking argument as optional

Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
marking argument as optional

Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
marking argument as optional

Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
@chrisiacovella
Copy link
Contributor Author

Ok, I think all comments are addressed. Read the docs is still failing because of the "unexpected" indents in some of the hoomd conversion files. Might need to do a separate PR to try to figure out why those are failing because nothing seems wrong.

Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

LGTM!

@daico007 daico007 merged commit ab6d54d into mosdef-hub:main Mar 14, 2023
@daico007 daico007 mentioned this pull request May 10, 2023
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.

2 participants