Deprecate buggy legacy implementation of kpoint generation in KpointsData #1015
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some methods of the KpointsData to analyze the Bravais lattice and set k-point paths based on that analysis, contain bugs for more complicated cell structures like monoclinic and triclinic cells. A proper implementation can be found in the Seekpath package. In prepartion of moving towards using this package, we deprecate the broken methods and move them to a legacy file in the aiida.tools.data.array.kpoints folder. For the time being the original methods will still refer to the original implementation with a deprecation warning that will be emitted.
This was done for consistency with the upcoming implementation of the Seekpath package to generate high-symmetry kpoints paths. The following name changes were applied: * get_special_points -> get_kpoints_path * set_kpoint_path -> get_explicit_kpoints_path Since the old interface in aiida.orm.data.array.kpoints still uses the old method names that just pipe through to these legacy functions in the tools subdirectory, all old behavior will remain the same.
…ethods There are two new methods in aiida.tools.data.array.kpoints: * get_kpoints_path * get_explicit_kpoints_path These functions will get a path along high-symmetry points and a list of explicit points along that high-symmetry path, respectively. By default they will use the Seekpath library, but the user can with the input 'method' toggle the use of the deprecated legacy implementation.
The return tuple is changed to return exactly the same tuple as the get_kpoints_path, with two more added return values: * explicit_kpoints: a list with explicit kpoints * labels: a list with the kpoint labels Also made sure that the cartesian input flag is passed properly to the calls of get_kpoints_path that the function makes
We copied to tests of the KpointsData class and changed them such as to go through the new legacy wrapper interface that replaced the deprecated KpointsData methods. This verifies that the wrappers for the 'legacy' method implementation still expose the same functionality and work the same
giovannipizzi
added a commit
to giovannipizzi/seekpath
that referenced
this pull request
Dec 22, 2017
conversion of spglib tuple to AiiDA and viceversa, renamed method from seekpath_tuple to spglib_tuple that is more correct and moved in a specific structure module, and added them in aiida.tools for easier importability.
unittest rather than AiiDATestCase)
improved missing existing documentation
giovannipizzi
approved these changes
Dec 22, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1001 and fixes #140
The methods
set_kpoints_pathandget_special_pointsof theKpointsDataclass can be used to automatically generate a list of (explicit) kpoints based on the symmetry of the cell lattice. These methods have been shown to be buggy and a more robust implementation is available in the package Seekpath. In this pull request, we deprecate the affected methods and move the implementation toaiida.tools.data.array.kpoints.legacy, changing the interface a bit to be more consistent. We also implement the AiiDA wrappers of Seekpath directly inaiida_corein theaiida.tools.data.array.kpoints.seekpathfile. We expose both methods with two general functions calledget_kpoints_pathandget_explicit_kpoints_pathin theaiida.tools.data.array.kpointsmodule. These can be called by the user and will by default use Seekpath, but can be toggled to use the legacy implementation with themethodinput flag.