aiidateam / qe-tools

A set of useful tools for Quantum ESPRESSO
MIT License
28 stars 13 forks source link

Add utility to convert cell into parameters. #36

Closed greschd closed 4 years ago

greschd commented 4 years ago

Blocked because it's based on #35.

New functionality:

Adds a get_parameters_from_cell function, which converts the cell into either the A, B, C, cosAB, cosAC, cosBC or the celldm(*) parameters. The function relies on ibrav being specified by the user. The existing get_cell_from_parameters is used to check the results - this detects cases where the given ibrav and cell do not match.

Naming:

The get_parameters_from_cell is put in a new generators module. The idea behind this name is that (input-)generators are opposite to the existing (output-)parsers. It's not a great naming scheme so better ideas are very welcome.

Scaffolding:

The pytest-cases library is used to create test cases, from the existing reference data for the parser.

giovannipizzi commented 4 years ago

Hi! Let me know if you want to rebase now that #35 is merged. Question: are type hints supported in the currently-supported version of python here?

Also, I like the idea that you ask the user to give a ibrav, it makes it a bit more robust I think.

But now, thinking to it, could you make a function that calls the function with all known ibravs, and returns which ones could work? and maybe one could sort them, and have a way to pick automatically the "first" one (being the one of highest symmetry, e.g. cubic rather than tetra, ortho, ...)? So you could provide an additional "automatic" interface that just gets the cell and does all the job, including detecting the best 'ibrav', in case the user does not care (I would still keep the current interface if you know which ibrav should be used).

greschd commented 4 years ago

Hi! Let me know if you want to rebase now that #35 is merged.

Done!

Question: are type hints supported in the currently-supported version of python here?

Yes, the only thing that's not supported is type annotations for variables - those need to be put into a comment, see e.g. https://github.com/greschd/qe-tools/blob/add_cell_to_parameters/qe_tools/generators/cell_conversion.py#L153

Also, I like the idea that you ask the user to give a ibrav, it makes it a bit more robust I think.

But now, thinking to it, could you make a function that calls the function with all known ibravs, and returns which ones could work? and maybe one could sort them, and have a way to pick automatically the "first" one (being the one of highest symmetry, e.g. cubic rather than tetra, ortho, ...)? So you could provide an additional "automatic" interface that just gets the cell and does all the job, including detecting the best 'ibrav', in case the user does not care (I would still keep the current interface if you know which ibrav should be used).

Hmm.. I guess that could work. What I worry a bit is that it might be a bit too easy to use incorrectly. The limitation of this approach is that it will only give the correct (physically) ibrav when the input cell matches one of the orientations QE can create. If the user supplies e.g. a hexagonal cell in the wrong orientation, it will just fall back on triclinic. That could be hard to spot for the user, so I'd prefer using something like spglib to figure out the Bravais lattice independent of orientation.

It could be implemented as

The advantage there would be that you can still throw an error if it's just the orientation that is wrong. Probably makes it slightly outside the scope of this PR, though.

greschd commented 4 years ago

Note also I've written numpy-style docstrings here. Since we don't have a doc yet it doesn't matter, but this requires sphinxcontrib.napoleon to be formatted properly.

greschd commented 4 years ago

Yeah, would be good to put in on PyPI in some way. Maybe we can put a release candidate 2.0.0.rc1 out, and if there are no issues with using it in aiida-quantumespresso or elsewhere we do the release proper?

giovannipizzi commented 4 years ago

Ok for me to release a rc1! You can even go ahead, you should have all credentials (so it's also a good check that everything works also for you). I guess you know very well how to do it, I typically use these instructions: https://github.com/aiidateam/qe-tools/wiki/How-to-upload-a-new-package-on-PyPi (so maybe you also have some suggestion ;-) ).

Maybe we should just address your #37 first

greschd commented 4 years ago

Great, I'll do that after #38 is merged.

Instructions look good. As an additional step, I usually install + test each of the dist files on a clean env, to make sure MANIFEST etc. are all fine. Ideally the CI captures that (e.g. as we do it in aiida-wannier90 https://github.com/greschd/aiida-wannier90/blob/develop/.travis-data/install_script.sh#L16), but running it by hand also works.

giovannipizzi commented 4 years ago

Thanks! #38 is now merged, probably one needs to update the version before releasing. Thanks also for the tip - maybe you could update the wiki when you do the release? Thanks!