[WIP] Introduce more intuitive method names in Data classes and reduce required input parameter for TrajectoryData.set_trajectory() method #2310
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
Change tests for dataclasses (StructureData, TrajectoryData) to call the renamed functions .get_structures() (former ._get_aiida_structures()) and .get_cif() (former ._get_cif())
Change TrajectoryData to make passing stepids and cells optional. While nothing will be stored for cells if not given a consecutive sequence will be automatically assigned to stepids if missing from the inputs.
As proposed in issue aiidateam#201 symbols are now stored in a TrajectoryData attribute rather than an array to simplify the query for these symbols
Move set of pylint options inside CifData class. Disable check for too-many-public-methods complaining about 21/20 public methods after renaming _get_aiida_structure -> get_structure making get_structure() method public
Make class methods .is_alloy() and .has_vacancies() for StrutureData and Kind classes accessible as class properties .is_alloy and .has_vacancies
giovannipizzi
requested changes
Dec 7, 2018
There was a problem hiding this comment.
Beside the comments above:
- rename
is_hiddento hidden - we would need a data migration before merging, for the symbols going from an array to a attribute.
If you do all other changes, we can then fork off your branch and implement the migration ourselves. Thanks!
giovannipizzi
changed the title
In function _internal_validate the 'symbols' list containing atomic names is checked for being 1D using the shape of the array. However, each entry of this list is ensured to be of type six.string_types somewhere upstream in this function which would already have triggered if the list is not purely 1D (contains one or more lists as entry somewhere)
Had to reformat the warning.warn() function arguments to get pylint invoke the set 'disable=no-member' option.
giovannipizzi
changed the base branch from
provenance_redesign
to
fix_201_method_names_for_data
|
Squash-merged into a working branch to keep attribution to @astamminger and we'll continue working from there |
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.
The changes in this pull request address issue #201 providing more intuitive method names. I clustered the changes into three major topics and would be glad to hear your opinion
1. Renamed functions
ArrayData.iterarrays()->.get_iterarrays()TrajectoryData._get_cif()->.get_cif()._get_aiida_structure->.get_structure()StructureData._get_cif()->.get_cif()Code.full_text_info()->.get_full_text_info()Functions have been renamed and the tests have been adopted accordingly. (Deprecation warnings have been added to all "old" functions)
2. Methods changed to properties
Code.is_hidden()->.hiddenRemoteData.is_empty()->.is_emptyStructureData,Kind.is_alloy()->.is_alloy.has_vacancies()->.has_vacanciesIdea was that these values are rather "attributes" of these classes and it is more intuitive to treat them as properties rather than actual methods. Question here is: Is this OK and how to exactly deal with this since we cannot simply add deprecation warnings.
3. Changes in
TrajectoryData.set_trajectory():Input parameters
stepidandcellsare now optional and the implemented behavior is as follows:cellsparameter is missing: Nothing will be stored for cells.stepidsparameter is missing:stepidsdefaults to the consecutive sequence [0,1,...,len(positions)-1]Element symbols passed to the
set_trajectory()method are now passed as a list (instead of a numpy.ndarray) and are stored in theTrajectoryData.symbolsattribute (as requested in #201 to simplify querying)