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 new format for OpenMX #585

Merged
merged 12 commits into from
Dec 6, 2023
Merged

Conversation

shigeandtomo
Copy link
Contributor

@shigeandtomo shigeandtomo commented Dec 3, 2023

Dear developers,

I added a new format key, openmx to read output files from OpenMX. I wrote the codes with reference to the qe/cp/traj format ones.

Thank you in advance.

@shigeandtomo shigeandtomo changed the title Add the possibility of reading OpenMX output files Add new format for OpenMX Dec 3, 2023
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

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

Comparison is base (18a0ed5) 82.81% compared to head (88a32d3) 83.13%.
Report is 9 commits behind head on devel.

Files Patch % Lines
dpdata/openmx/omx.py 92.75% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            devel     #585      +/-   ##
==========================================
+ Coverage   82.81%   83.13%   +0.32%     
==========================================
  Files          73       76       +3     
  Lines        6580     6766     +186     
==========================================
+ Hits         5449     5625     +176     
- Misses       1131     1141      +10     

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

Copy link
Member

@njzjz njzjz left a comment

Choose a reason for hiding this comment

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

The pre-commit check should be passed before merging. See https://docs.deepmodeling.com/projects/deepmd/en/master/development/coding-conventions.html#run-scripts-to-check-the-code

It's not automatically fixed because you submit the PR from the master branch.

from dpdata.format import Format

@Format.register("openmx")
class OPENMXFormat(Format):
Copy link
Member

@njzjz njzjz Dec 3, 2023

Choose a reason for hiding this comment

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

I suggest adding some documentation for this new format. For example, a link to external documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@njzjz Thank you for the reply.

I have a question. What is the “documentation" in your message?

For example, on page 36 of the dpdata documentation, there are description about qe/cp/traj, right? Do you meant that a similar description should be added for openmx?

Or is it sufficient to simply attach a link to the DFT package OpenMX, like this?

I look forward to your advice.

Copy link
Member

@njzjz njzjz Dec 5, 2023

Choose a reason for hiding this comment

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

Something like this, e.g. what openmx is and what this format is.

class ASEStructureFormat(Format):
"""Format for the `Atomic Simulation Environment <https://wiki.fysik.dtu.dk/ase/>`_ (ase).
ASE supports parsing a few dozen of data formats. As described in i
`the documentation <ihttps://wiki.fysik.dtu.dk/ase/ase/io/io.html>`_,
many of these formats can be determined automatically.
Use the `ase_fmt` keyword argument to supply the format if
automatic detection fails.
"""

Btw, it may be better to rename it to openmx/out (or something like this) or add an alias to show this is the output but not the input. The input file can also be a format.

Copy link
Contributor Author

@shigeandtomo shigeandtomo Dec 5, 2023

Choose a reason for hiding this comment

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

@njzjz Thank you.

I maked out what you meant. So, I added some documentation for this new format, and changed keyword from openmx to openmx/out.

@wanghan-iapcm wanghan-iapcm merged commit ec08ebb into deepmodeling:devel Dec 6, 2023
9 checks passed
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.

4 participants