Skip to content
This repository has been archived by the owner on Jun 19, 2024. It is now read-only.

Addition of connectivity, radii and colors. #2

Merged
merged 16 commits into from
Jan 2, 2024
Merged

Conversation

phohenberger
Copy link
Collaborator

No description provided.

@phohenberger phohenberger requested a review from PythonFZ January 2, 2024 16:37
@codecov-commenter
Copy link

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (41957ca) 100.00% compared to head (bd0bea3) 88.75%.

Files Patch % Lines
znframe/bonds/__init__.py 64.00% 18 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              main       #2       +/-   ##
============================================
- Coverage   100.00%   88.75%   -11.25%     
============================================
  Files            3        4        +1     
  Lines           93      160       +67     
============================================
+ Hits            93      142       +49     
- Misses           0       18       +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@PythonFZ PythonFZ left a comment

Choose a reason for hiding this comment

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

Looks good! There is only one change, where I think the conversion from graph to list should be handled by a converter instead of the inside the post init

Comment on lines +68 to +71
if self.connectivity is None:
ase_bond_calculator = ASEComputeBonds()
self.connectivity = ase_bond_calculator.build_graph(self.to_atoms())
self.connectivity = ase_bond_calculator.get_bonds(self.connectivity)
Copy link
Member

Choose a reason for hiding this comment

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

Could this be a converter instead of being part of the __post_init__

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not simply done as you need an ase.Atoms-object to calculate the bonds. And it is not possible to convert the frame to an ase.Atoms while the initialization is still in progess.

Copy link
Member

@PythonFZ PythonFZ left a comment

Choose a reason for hiding this comment

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

Looks good

@phohenberger phohenberger merged commit e3ff8d4 into main Jan 2, 2024
4 checks passed
@phohenberger phohenberger deleted the connectivity branch January 2, 2024 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants