-
Notifications
You must be signed in to change notification settings - Fork 135
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
improve: ase try to get virials from different sources #660
base: devel
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThe recent update to the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedGitHub Check Runs (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #660 +/- ##
==========================================
+ Coverage 84.06% 84.07% +0.01%
==========================================
Files 81 81
Lines 7122 7127 +5
==========================================
+ Hits 5987 5992 +5
Misses 1135 1135 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #660 will not alter performanceComparing Summary
|
# try to get virials from different sources | ||
virials = atoms.info.get("virial") | ||
if virials is None: | ||
virials = atoms.info.get("virials") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear why it has two different keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fallback strategy as users may use either virial
or virials
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is necessary if users are expected to do something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to search in the code base but find no place to add comment for this.
I don't think we need add extra documents for this as dpdata
is supposed to be able to find virial
for user automatically. My patch doesn't introduce any compatibility issue, it just make the ase plugin more robust to find virial
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It finds data from non-standard keys, which needs documentation to avoid unexpected behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just add a comment in docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
# try to get virials from different sources | ||
virials = atoms.info.get("virial") | ||
if virials is None: | ||
virials = atoms.info.get("virials") | ||
if virials is None: | ||
try: | ||
stress = atoms.get_stress(False) | ||
except PropertyNotImplementedError: | ||
pass | ||
else: | ||
virials = np.array([-atoms.get_volume() * stress]) | ||
if virials is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling the PropertyNotImplementedError
for stress calculation more gracefully.
The current implementation passes silently if PropertyNotImplementedError
is raised when trying to get stress (lines 104-105). This could lead to virials
being None
without any indication of why, which might confuse users. Consider logging a warning or providing a fallback mechanism.
- except PropertyNotImplementedError:
- pass
+ except PropertyNotImplementedError as e:
+ logging.warning(f"Failed to compute stress due to: {str(e)}")
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
# try to get virials from different sources | |
virials = atoms.info.get("virial") | |
if virials is None: | |
virials = atoms.info.get("virials") | |
if virials is None: | |
try: | |
stress = atoms.get_stress(False) | |
except PropertyNotImplementedError: | |
pass | |
else: | |
virials = np.array([-atoms.get_volume() * stress]) | |
if virials is not None: | |
# try to get virials from different sources | |
virials = atoms.info.get("virial") | |
if virials is None: | |
virials = atoms.info.get("virials") | |
if virials is None: | |
try: | |
stress = atoms.get_stress(False) | |
except PropertyNotImplementedError as e: | |
logging.warning(f"Failed to compute stress due to: {str(e)}") | |
else: | |
virials = np.array([-atoms.get_volume() * stress]) | |
if virials is not None: |
dpdata/plugins/ase.py
Outdated
"""Convert System to ASE Atoms object. | ||
|
||
Note that this method will try to load virials from the following sources: | ||
- atoms.info['virial'] | ||
- atoms.info['virials'] | ||
- converted from stress tensor | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the method documentation to reflect the new virial retrieval logic.
The documentation in the to_labeled_system
method (lines 170-176) mentions the sources from which virials are loaded, but it does not reflect the new logic added in from_labeled_system
for obtaining virials from different sources and calculating them from stress. Consider updating the documentation to maintain consistency and clarity.
- - converted from stress tensor
+ - converted from stress tensor if other sources are unavailable
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
"""Convert System to ASE Atoms object. | |
Note that this method will try to load virials from the following sources: | |
- atoms.info['virial'] | |
- atoms.info['virials'] | |
- converted from stress tensor | |
""" | |
"""Convert System to ASE Atoms object. | |
Note that this method will try to load virials from the following sources: | |
- atoms.info['virial'] | |
- atoms.info['virials'] | |
- converted from stress tensor if other sources are unavailable | |
""" |
dd2ae5f
to
451bd0d
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Note that this method will try to load virials from the following sources: | ||
- atoms.info['virial'] | ||
- atoms.info['virials'] | ||
- converted from stress tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the method documentation to reflect the new virial retrieval logic.
- - converted from stress tensor
+ - converted from stress tensor if other sources are unavailable
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Note that this method will try to load virials from the following sources: | |
- atoms.info['virial'] | |
- atoms.info['virials'] | |
- converted from stress tensor | |
Note that this method will try to load virials from the following sources: | |
- atoms.info['virial'] | |
- atoms.info['virials'] | |
- converted from stress tensor if other sources are unavailable |
Ensure the documentation accurately describes all sources for virials.
- - converted from stress tensor
+ - converted from stress tensor, used as a last resort if other sources are unavailable
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Note that this method will try to load virials from the following sources: | |
- atoms.info['virial'] | |
- atoms.info['virials'] | |
- converted from stress tensor | |
Note that this method will try to load virials from the following sources: | |
- atoms.info['virial'] | |
- atoms.info['virials'] | |
- converted from stress tensor, used as a last resort if other sources are unavailable |
except PropertyNotImplementedError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle PropertyNotImplementedError
more gracefully to inform the user.
- except PropertyNotImplementedError:
- pass
+ except PropertyNotImplementedError as e:
+ logging.warning(f"Failed to compute stress due to: {str(e)}")
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
except PropertyNotImplementedError: | |
pass | |
except PropertyNotImplementedError as e: | |
logging.warning(f"Failed to compute stress due to: {str(e)}") |
Note that this method will try to load virials from the following sources: | ||
- atoms.info['virial'] | ||
- atoms.info['virials'] | ||
- converted from stress tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not correctly rendered. All three items in the list are shown in one line. @coderabbitai Fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@njzjz, I've adjusted the docstring to ensure the list items are rendered correctly. Here's the corrected version:
def from_labeled_system(self, atoms: ase.Atoms, **kwargs) -> dict:
"""Convert ase.Atoms to a LabeledSystem. Energies and forces
are calculated by the calculator.
Note that this method will try to load virials from the following sources:
- atoms.info['virial']
- atoms.info['virials']
- converted from stress tensor
Parameters
----------
atoms : ase.Atoms
an ASE Atoms, containing a structure
**kwargs : dict
other parameters
Returns
-------
dict
data dict
Raises
------
RuntimeError
ASE will raise RuntimeError if the atoms does not
have a calculator
"""
This should ensure that the list is displayed correctly in any markdown-rendering context. Please let me know if there are any more adjustments needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a blank line needs to be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@njzjz, I've added the requested blank line to the docstring. Here's the updated version:
def from_labeled_system(self, atoms: ase.Atoms, **kwargs) -> dict:
"""Convert ase.Atoms to a LabeledSystem. Energies and forces
are calculated by the calculator.
Note that this method will try to load virials from the following sources:
- atoms.info['virial']
- atoms.info['virials']
- converted from stress tensor
Parameters
----------
atoms : ase.Atoms
an ASE Atoms, containing a structure
**kwargs : dict
other parameters
Returns
-------
dict
data dict
Raises
------
RuntimeError
ASE will raise RuntimeError if the atoms does not
have a calculator
"""
This should ensure that the list is displayed correctly in any markdown-rendering context. Please let me know if this looks good or if any further adjustments are needed!
Summary by CodeRabbit