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

Allow explicitly specifying 'ibrav'. #503

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

greschd
Copy link
Member

@greschd greschd commented Apr 17, 2020

Fixes #347.

If an ibrav value other than zero is specified in the SYSTEM inputs, the cell of the input structure is converted into the appropriate A, B, C, cosAB, cosAC, cosBC values. As a check, the cell is reconstructed using qe_tools. The cells are compared element-wise, with an absolute tolerance controlled by the IBRAV_CELL_TOLERANCE setting.

Still needs testing and documentation. No need to check the correctness yet, but I'm soliciting feedback on the interface.

Note also: This adds numpy and scipy as explicit dependencies. They're implicit dependencies already, so I don't think that should be problematic.

TODO:

  • add tests for the conversion between cell and parameters these will go into qe-tools directly
  • document the explicit ibrav feature
  • add test for immigration, possibly add conversion from parameters to cell
  • move cell -> parameter conversion to qe_tools?

@greschd
Copy link
Member Author

greschd commented Apr 17, 2020

image
Ah, Python2, loving it 😅

@greschd greschd force-pushed the feature/allow_explicit_ibrav branch 2 times, most recently from eace20f to f0d21b6 Compare April 20, 2020 10:01
@greschd
Copy link
Member Author

greschd commented Apr 20, 2020

@giovannipizzi should the "cell to parameter" conversion go into qe_tools?

One thing to consider here is that there are no checks in that function itself that the cell conforms to the specified ibrav - that is done later. If we want to move it into qe_tools and make it a publicly-available function, it should probably do some checking itself.

@greschd greschd force-pushed the feature/allow_explicit_ibrav branch from f0d21b6 to edf2532 Compare April 20, 2020 11:57
@giovannipizzi
Copy link
Member

@greschd yes, maybe as they are general they can be put into qe-tools?

In this case I agree testing should be done by qe-tools, and raise there.
If we want to do it, I suggest this could be a good moment to release a v2, with py3 only support, and maybe changing a bit the interface? (I don't like so much the way you initialise things in qe-tools now, where it guesses what you want depending if the string you pass is a valid absolute path, otherwise it falls back to parse the string itself). Suggestions welcome there.

@greschd
Copy link
Member Author

greschd commented Apr 24, 2020

I suggest this could be a good moment to release a v2, with py3 only support, and maybe changing a bit the interface?

AFAIK aiida-quantumespesso still maintains py2 compatibility for now. How about we proceed with it here, then polish it for inclusion in qe_tools and replace it here once that's released and compatible and aiida-quantumespresso drops py2?

For inclusion in qe_tools, probably it should also be able to change between the celldm and A, B, ... output formats.

@sphuber
Copy link
Contributor

sphuber commented Apr 24, 2020

Indeed, we first want to release aiida-quantumespresso==3.0.0 with python 2 support, because that is the first official release that is both Python 2/3 and AiiDA 1.0 compatibile. However, soon after I was planning to drop Python 2 and require aiida-core>=1.2 as I need certain features from those releases for advanced functionality. We will then only provide minimum support for 3.0.0 series and just continue with what will become 3.1 or 4.0, to be decided

@greschd
Copy link
Member Author

greschd commented Apr 24, 2020

Ok, what's your opinion on the interface of this PR, specifically the way specifying ibrav works with the single absolute tolerance on the cell?

If you're fine with this I will add tests and documentation.

@sphuber
Copy link
Contributor

sphuber commented Apr 24, 2020

I don't have the time for it today, unfortunately, sorry. I am live demo'ing AiiDA in two hours and then have to prepare a proposal. Will try to look at it soon

@greschd
Copy link
Member Author

greschd commented Apr 24, 2020

No worries, it's not an urgent thing. Good luck on the live demo!

@giovannipizzi
Copy link
Member

@greschd yeah sorry, you're right.
Then it's just a matter of moving the relevant code into qe-tools that still has py2+py3 compatibility.
Feel free to do a PR there! If you want I can also give you admin rights there so you can make releases yourself.

@giovannipizzi
Copy link
Member

I guess we should first release qe_tools before we can finalise this, right?

@greschd
Copy link
Member Author

greschd commented Jun 9, 2020

That's the plan 😄

@greschd greschd force-pushed the feature/allow_explicit_ibrav branch 2 times, most recently from 7db57eb to f7a7d2f Compare June 22, 2020 08:30
@greschd
Copy link
Member Author

greschd commented Jun 22, 2020

@sphuber what's the best place to put the documentation for this feature? Maybe a subsection in https://aiida-quantumespresso.readthedocs.io/en/latest/user_guide/get_started/examples/pw_tutorial.html#structure could work, but I'm not sure if there might be a better place.

@sphuber
Copy link
Contributor

sphuber commented Jun 22, 2020

To be honest, I haven't yet been able to look at the documentation and it badly needs to be improved I feel. So I don't have an opinion really. Just put it wherever you feel is best for now.

@greschd greschd force-pushed the feature/allow_explicit_ibrav branch from f7a7d2f to 7d87936 Compare June 24, 2020 10:08
@greschd greschd changed the title Allow explicitly specifying 'ibrav'. [BLOCKED] Allow explicitly specifying 'ibrav'. Jun 24, 2020
@greschd
Copy link
Member Author

greschd commented Jun 24, 2020

Ok, I've added a short paragraph in docs/source/user_guide/calculation_plugins/pw.rst and docs/source/user_guide/calculation_plugins/cp.rst.

@greschd greschd marked this pull request as ready for review June 24, 2020 10:10
@greschd greschd force-pushed the feature/allow_explicit_ibrav branch 2 times, most recently from 8cd83ac to 79a525e Compare July 30, 2020 09:52
@greschd greschd force-pushed the feature/allow_explicit_ibrav branch 2 times, most recently from 03a4d91 to e4d4c42 Compare October 8, 2020 08:36
@greschd greschd changed the title [BLOCKED] Allow explicitly specifying 'ibrav'. Allow explicitly specifying 'ibrav'. Oct 8, 2020
@greschd greschd force-pushed the feature/allow_explicit_ibrav branch from e4d4c42 to 0043c3a Compare October 8, 2020 08:40
@greschd
Copy link
Member Author

greschd commented Oct 8, 2020

Rebased on the latest develop and ready to go!

@greschd greschd force-pushed the feature/allow_explicit_ibrav branch 2 times, most recently from 31897da to bda0881 Compare October 8, 2020 10:35
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @greschd . Looking good. Just some minor suggestions and request to add a test.

aiida_quantumespresso/calculations/__init__.py Outdated Show resolved Hide resolved
setup.json Outdated Show resolved Hide resolved
tests/calculations/test_pw.py Show resolved Hide resolved
@greschd greschd force-pushed the feature/allow_explicit_ibrav branch 2 times, most recently from 1f56896 to f3c07b5 Compare October 8, 2020 13:08
If an 'ibrav' value other than zero is specified in the
SYSTEM inputs, the cell of the input structure is converted
into the appropriate A, B, C, cosAB, cosAC, cosBC values. As a
check, the cell is reconstructed using qe_tools. The cells are
compared element-wise, with an absolute tolerance controlled by
the 'IBRAV_CELL_TOLERANCE' setting.
@greschd greschd force-pushed the feature/allow_explicit_ibrav branch from f3c07b5 to 08f2f4f Compare October 8, 2020 13:18
@greschd greschd requested a review from sphuber October 8, 2020 13:22
@sphuber sphuber merged commit 5ca286b into aiidateam:develop Oct 8, 2020
@greschd greschd deleted the feature/allow_explicit_ibrav branch October 8, 2020 15:09
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.

Provide option to set an explicit ibrav value
3 participants