feat(extxyz): add unit convert and tag synonym matching#678
feat(extxyz): add unit convert and tag synonym matching#678SchrodingersCattt wants to merge 9 commits into
Conversation
📝 WalkthroughWalkthroughExtended XYZ parsing now reads optional unit metadata for energy, force, and stress, converts those values into dpdata internal units, and applies the factors during frame loading. New helper functions, fixtures, and tests cover supported units, default behavior, and unsupported-unit errors. ChangesExtXYZ unit conversion
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
for more information, see https://pre-commit.ci
Merging this PR will not alter performance
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #678 +/- ##
==========================================
+ Coverage 86.87% 86.93% +0.05%
==========================================
Files 89 90 +1
Lines 8268 8319 +51
==========================================
+ Hits 7183 7232 +49
- Misses 1085 1087 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| elif f_units == "kcal/mol/bohr": | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") * f_conv_au2eVperang |
There was a problem hiding this comment.
Remove redundant conditional block for kcal/mol/bohr.
The conditional block for kcal/mol/bohr is redundant and should be removed.
- elif f_units == "kcal/mol/bohr":
- info_dict["forces"] = (
- np.array([force_array]).astype("float32") * f_conv_au2eVperang
- )Committable suggestion was skipped due to low confidence.
| else: | ||
| raise ValueError("No valid energy field found in field_dict.") | ||
| except KeyError: | ||
| raise ValueError("Error while accessing energy fields in field_dict.") |
There was a problem hiding this comment.
Raise exceptions with raise ... from None.
Within an except clause, raise exceptions with raise ... from None to distinguish them from errors in exception handling.
- raise ValueError("Error while accessing energy fields in field_dict.")
+ raise ValueError("Error while accessing energy fields in field_dict.") from NoneCommittable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ValueError("Error while accessing energy fields in field_dict.") | |
| raise ValueError("Error while accessing energy fields in field_dict.") from None |
Tools
Ruff
252-252: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| else: | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| except: |
There was a problem hiding this comment.
Avoid using bare except statements.
Using bare except statements can catch unexpected errors and make debugging difficult. Specify the exception type or use except Exception as e to handle specific exceptions.
- except:
+ except KeyError:Committable suggestion was skipped due to low confidence.
Tools
Ruff
283-283: Do not use bare
except(E722)
| try: | ||
| if f_units == "kcal/mol/angstrom": | ||
| <<<<<<< HEAD | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") * f_conv_kcalpermolperang2eVperang | ||
| elif f_units == "hartree/angstrom" or f_units == "hartree/ang" or f_units == "hartree/ang.": | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") * f_conv_auperang2eVperang | ||
| elif f_units == "kcal/mol/bohr": | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") * f_conv_kcalpermolperbohr2eVperang | ||
| elif f_units == "kcal/mol/bohr": | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") * f_conv_au2eVperang | ||
| elif f_units == "ev/angstrom" or f_units == "ev/ang" or f_units == "ev/ang.": | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| else: info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| except Exception: | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| ======= | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") | ||
| * f_conv_kcalpermolperang2eVperang | ||
| ) | ||
| elif ( | ||
| f_units == "hartree/angstrom" | ||
| or f_units == "hartree/ang" | ||
| or f_units == "hartree/ang." | ||
| ): | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") * f_conv_auperang2eVperang | ||
| ) | ||
| elif f_units == "kcal/mol/bohr": | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") | ||
| * f_conv_kcalpermolperbohr2eVperang | ||
| ) | ||
| elif f_units == "kcal/mol/bohr": | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") * f_conv_au2eVperang | ||
| ) | ||
| elif ( | ||
| f_units == "ev/angstrom" or f_units == "ev/ang" or f_units == "ev/ang." | ||
| ): | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| else: | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| except: | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| >>>>>>> origin/devel |
There was a problem hiding this comment.
Unit conversion for forces and redundant conditional block.
The new logic for force unit conversion is correctly implemented but contains a redundant conditional block for kcal/mol/bohr and uses a bare except statement.
- elif f_units == "kcal/mol/bohr":
- info_dict["forces"] = (
- np.array([force_array]).astype("float32") * f_conv_au2eVperang
- )
- except:
- info_dict["forces"] = np.array([force_array]).astype("float32")
+ except KeyError as e:
+ raise ValueError("Error while accessing force fields in field_dict.") from eCommittable suggestion was skipped due to low confidence.
Tools
Ruff
255-256: SyntaxError: Expected an indented block after
ifstatement
256-256: SyntaxError: Expected
exceptorfinallyaftertryblock
256-256: SyntaxError: Expected a statement
256-256: SyntaxError: Expected a statement
256-256: SyntaxError: Expected a statement
257-257: SyntaxError: Unexpected indentation
258-258: SyntaxError: unindent does not match any outer indentation level
258-258: SyntaxError: Expected a statement
258-258: SyntaxError: Invalid annotated assignment target
258-259: SyntaxError: Expected an expression
259-259: SyntaxError: Unexpected indentation
260-260: SyntaxError: unindent does not match any outer indentation level
260-260: SyntaxError: Expected a statement
260-260: SyntaxError: Invalid annotated assignment target
260-261: SyntaxError: Expected an expression
261-261: SyntaxError: Unexpected indentation
262-262: SyntaxError: unindent does not match any outer indentation level
262-262: SyntaxError: Expected a statement
262-262: SyntaxError: Invalid annotated assignment target
262-263: SyntaxError: Expected an expression
263-263: SyntaxError: Unexpected indentation
264-264: SyntaxError: unindent does not match any outer indentation level
264-264: SyntaxError: Expected a statement
264-264: SyntaxError: Invalid annotated assignment target
264-265: SyntaxError: Expected an expression
265-265: SyntaxError: Unexpected indentation
266-266: SyntaxError: unindent does not match any outer indentation level
266-266: SyntaxError: Expected a statement
266-266: SyntaxError: Expected a statement
267-267: SyntaxError: Unexpected indentation
267-267: SyntaxError: Expected a statement
267-268: SyntaxError: Expected an expression
268-268: SyntaxError: Unexpected indentation
269-269: SyntaxError: Expected a statement
269-269: SyntaxError: Expected a statement
269-269: SyntaxError: Expected a statement
269-269: SyntaxError: Expected a statement
269-270: SyntaxError: Expected a statement
270-270: SyntaxError: Unexpected indentation
274-274: SyntaxError: unindent does not match any outer indentation level
274-274: SyntaxError: Expected a statement
275-275: SyntaxError: Invalid annotated assignment target
278-279: SyntaxError: Expected an expression
279-279: SyntaxError: Unexpected indentation
282-282: SyntaxError: unindent does not match any outer indentation level
282-282: SyntaxError: Expected a statement
282-282: SyntaxError: Invalid annotated assignment target
282-283: SyntaxError: Expected an expression
283-283: SyntaxError: Unexpected indentation
287-287: SyntaxError: unindent does not match any outer indentation level
287-287: SyntaxError: Expected a statement
287-287: SyntaxError: Invalid annotated assignment target
287-288: SyntaxError: Expected an expression
288-288: SyntaxError: Unexpected indentation
291-291: SyntaxError: unindent does not match any outer indentation level
291-291: SyntaxError: Expected a statement
292-292: SyntaxError: Invalid annotated assignment target
293-294: SyntaxError: Expected an expression
294-294: SyntaxError: Unexpected indentation
295-295: SyntaxError: unindent does not match any outer indentation level
295-295: SyntaxError: Expected a statement
295-295: SyntaxError: Expected a statement
295-296: SyntaxError: Expected a statement
296-296: SyntaxError: Unexpected indentation
297-297: SyntaxError: unindent does not match any outer indentation level
297-297: SyntaxError: Expected a statement
297-297: SyntaxError: Expected a statement
297-298: SyntaxError: Expected a statement
298-298: SyntaxError: Unexpected indentation
299-299: SyntaxError: Expected a statement
299-299: SyntaxError: Expected a statement
299-299: SyntaxError: Expected a statement
299-299: SyntaxError: Expected a statement
| try: | ||
| e_units = np.array([field_dict["energy-unit"].lower()]) | ||
| f_units = np.array([field_dict["force-unit"].lower()]) | ||
| except Exception: | ||
| pass | ||
| # print('No units information contained.') | ||
|
|
There was a problem hiding this comment.
Proper handling of missing unit information.
The try-except block for handling missing unit information should raise an exception or log a warning if units are not found, rather than passing silently.
- except Exception:
- pass
- # print('No units information contained.')
+ except KeyError as e:
+ raise ValueError("No units information contained.") from eCommittable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| e_units = np.array([field_dict["energy-unit"].lower()]) | |
| f_units = np.array([field_dict["force-unit"].lower()]) | |
| except Exception: | |
| pass | |
| # print('No units information contained.') | |
| try: | |
| e_units = np.array([field_dict["energy-unit"].lower()]) | |
| f_units = np.array([field_dict["force-unit"].lower()]) | |
| except KeyError as e: | |
| raise ValueError("No units information contained.") from e |
| try: | ||
| possible_fields = [ | ||
| "energy", | ||
| "energies", | ||
| "Energies", | ||
| "potential-energy.energy", | ||
| "Energy", | ||
| ] | ||
| for key in possible_fields: | ||
| if key in field_dict: | ||
| info_dict["energies"] = np.array( | ||
| [field_dict[key]], dtype="float32" | ||
| ) | ||
| break | ||
| else: | ||
| raise ValueError("No valid energy field found in field_dict.") | ||
| except KeyError: | ||
| raise ValueError("Error while accessing energy fields in field_dict.") | ||
|
|
There was a problem hiding this comment.
Improved handling of possible energy fields and raising exceptions.
The changes improve handling of possible energy fields and raising exceptions. Use raise ... from None to distinguish exceptions from errors in exception handling.
- raise ValueError("Error while accessing energy fields in field_dict.")
+ raise ValueError("Error while accessing energy fields in field_dict.") from NoneCommittable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| possible_fields = [ | |
| "energy", | |
| "energies", | |
| "Energies", | |
| "potential-energy.energy", | |
| "Energy", | |
| ] | |
| for key in possible_fields: | |
| if key in field_dict: | |
| info_dict["energies"] = np.array( | |
| [field_dict[key]], dtype="float32" | |
| ) | |
| break | |
| else: | |
| raise ValueError("No valid energy field found in field_dict.") | |
| except KeyError: | |
| raise ValueError("Error while accessing energy fields in field_dict.") | |
| try: | |
| possible_fields = [ | |
| "energy", | |
| "energies", | |
| "Energies", | |
| "potential-energy.energy", | |
| "Energy", | |
| ] | |
| for key in possible_fields: | |
| if key in field_dict: | |
| info_dict["energies"] = np.array( | |
| [field_dict[key]], dtype="float32" | |
| ) | |
| break | |
| else: | |
| raise ValueError("No valid energy field found in field_dict.") | |
| except KeyError: | |
| raise ValueError("Error while accessing energy fields in field_dict.") from None |
Tools
Ruff
252-252: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
| try: | ||
| if f_units == "kcal/mol/angstrom": | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") | ||
| * f_conv_kcalpermolperang2eVperang | ||
| ) | ||
| elif ( | ||
| f_units == "hartree/angstrom" | ||
| or f_units == "hartree/ang" | ||
| or f_units == "hartree/ang." | ||
| ): | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") * f_conv_auperang2eVperang | ||
| ) | ||
| elif f_units == "kcal/mol/bohr": | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") | ||
| * f_conv_kcalpermolperbohr2eVperang | ||
| ) | ||
| elif f_units == "kcal/mol/bohr": | ||
| info_dict["forces"] = ( | ||
| np.array([force_array]).astype("float32") * f_conv_au2eVperang | ||
| ) | ||
| elif ( | ||
| f_units == "ev/angstrom" or f_units == "ev/ang" or f_units == "ev/ang." | ||
| ): | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| else: | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| except Exception: |
There was a problem hiding this comment.
Unit conversion for forces and redundant conditional block.
The new logic for force unit conversion is correctly implemented but contains a redundant conditional block for kcal/mol/bohr and uses a bare except statement.
- elif f_units == "kcal/mol/bohr":
- info_dict["forces"] = (
- np.array([force_array]).astype("float32") * f_conv_au2eVperang
- )
- except:
- info_dict["forces"] = np.array([force_array]).astype("float32")
+ except KeyError as e:
+ raise ValueError("Error while accessing force fields in field_dict.") from eCommittable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| if f_units == "kcal/mol/angstrom": | |
| info_dict["forces"] = ( | |
| np.array([force_array]).astype("float32") | |
| * f_conv_kcalpermolperang2eVperang | |
| ) | |
| elif ( | |
| f_units == "hartree/angstrom" | |
| or f_units == "hartree/ang" | |
| or f_units == "hartree/ang." | |
| ): | |
| info_dict["forces"] = ( | |
| np.array([force_array]).astype("float32") * f_conv_auperang2eVperang | |
| ) | |
| elif f_units == "kcal/mol/bohr": | |
| info_dict["forces"] = ( | |
| np.array([force_array]).astype("float32") | |
| * f_conv_kcalpermolperbohr2eVperang | |
| ) | |
| elif f_units == "kcal/mol/bohr": | |
| info_dict["forces"] = ( | |
| np.array([force_array]).astype("float32") * f_conv_au2eVperang | |
| ) | |
| elif ( | |
| f_units == "ev/angstrom" or f_units == "ev/ang" or f_units == "ev/ang." | |
| ): | |
| info_dict["forces"] = np.array([force_array]).astype("float32") | |
| else: | |
| info_dict["forces"] = np.array([force_array]).astype("float32") | |
| except Exception: | |
| try: | |
| if f_units == "kcal/mol/angstrom": | |
| info_dict["forces"] = ( | |
| np.array([force_array]).astype("float32") | |
| * f_conv_kcalpermolperang2eVperang | |
| ) | |
| elif ( | |
| f_units == "hartree/angstrom" | |
| or f_units == "hartree/ang" | |
| or f_units == "hartree/ang." | |
| ): | |
| info_dict["forces"] = ( | |
| np.array([force_array]).astype("float32") * f_conv_auperang2eVperang | |
| ) | |
| elif f_units == "kcal/mol/bohr": | |
| info_dict["forces"] = ( | |
| np.array([force_array]).astype("float32") | |
| * f_conv_kcalpermolperbohr2eVperang | |
| ) | |
| elif ( | |
| f_units == "ev/angstrom" or f_units == "ev/ang" or f_units == "ev/ang." | |
| ): | |
| info_dict["forces"] = np.array([force_array]).astype("float32") | |
| else: | |
| info_dict["forces"] = np.array([force_array]).astype("float32") | |
| except KeyError as e: | |
| raise ValueError("Error while accessing force fields in field_dict.") from e |
| used_colomn += field_length | ||
| continue | ||
| except Exception as e: | ||
| print("unknown field {}".format(kv_dict["key"]), e) |
There was a problem hiding this comment.
What is the reason not to raise an error?
| ] | ||
| used_colomn += field_length | ||
| continue | ||
| except Exception as e: |
There was a problem hiding this comment.
What is the error it is expected to catch?
| """ | ||
| info_dict["energies"] = np.array([field_dict["energy"]]).astype("float32") | ||
| info_dict["forces"] = np.array([force_array]).astype("float32") | ||
| """ |
There was a problem hiding this comment.
If it is not used anymore, it should be removed.
| info_dict["energies"] = np.array([field_dict["energy"]]).astype( | ||
| "float32" | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
What is the expected error to catch here? It's hard to read.
| ) | ||
| elif e_units == "ev": | ||
| info_dict["energies"] = np.array([field_dict["energy"]]).astype( | ||
| "float32" |
njzjz
left a comment
njzjz
left a comment
There was a problem hiding this comment.
The unit test should be added to cover the cases in this PR.
SchrodingersCattt
changed the title
… stress - New module dpdata/formats/xyz/_unit_convert.py with unit alias maps and _get_unit_factor() helper that delegates to dpdata.unit converters - Support energy-unit, force-unit, stress-unit headers in extxyz files - Convert to dpdata internal units (eV, eV/angstrom, eV/angstrom^3) - Raise ValueError for unsupported units instead of silent pass - Use float64 throughout instead of float32 - Add _parse_force_unit() for composite unit strings (e.g. kcal/mol/angstrom) - Add stress_factor parameter to _parse_stress_to_virials() - Add comprehensive tests for unit conversion module and end-to-end Resolves reviewer feedback from PR deepmodeling#678: - No more bare except / silent error swallowing - No more dead commented-out code - No more float32 precision truncation - Full test coverage for unit conversion paths
# Conflicts: # dpdata/xyz/quip_gap_xyz.py
… stress - New module dpdata/formats/xyz/_unit_convert.py with unit alias maps and _get_unit_factor() helper that delegates to dpdata.unit converters - Support energy-unit, force-unit, stress-unit headers in extxyz files - Convert to dpdata internal units (eV, eV/angstrom, eV/angstrom^3) - Raise ValueError for unsupported units instead of silent pass - Use float64 throughout instead of float32 - Add _parse_force_unit() for composite unit strings (e.g. kcal/mol/angstrom) - Add stress_factor parameter to _parse_stress_to_virials() - Add comprehensive tests for unit conversion module and end-to-end Resolves reviewer feedback from PR deepmodeling#678: - No more bare except / silent error swallowing - No more dead commented-out code - No more float32 precision truncation - Full test coverage for unit conversion paths
dosubot
Bot
added
size:L
2 participants
Many datasets using the
exyzformat have different formats for units and tags. This PR aims to address the situation with multiple possible datasets.Summary by CodeRabbit
New Features
Improvements
Tests