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

Added element X to the elements list in order to support unknown species #1613

Merged
merged 5 commits into from
Jun 7, 2018

Conversation

espenfl
Copy link
Contributor

@espenfl espenfl commented May 31, 2018

Can be useful in parsing scenarios when the element is now known, or in cases where one does not explicitly handle atoms (electrons, orbital calculations etc.). It is then also possible to detect element X down the line and request user for additional information or do automatic corrections, if needed.

@codecov-io
Copy link

codecov-io commented May 31, 2018

Codecov Report

Merging #1613 into develop will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1613      +/-   ##
===========================================
+ Coverage    57.14%   57.17%   +0.02%     
===========================================
  Files          273      273              
  Lines        33773    33773              
===========================================
+ Hits         19300    19309       +9     
+ Misses       14473    14464       -9
Impacted Files Coverage Δ
aiida/orm/data/structure.py 78.21% <ø> (ø) ⬆️
aiida/common/constants.py 100% <ø> (ø) ⬆️
aiida/backends/djsite/db/models.py 75.97% <0%> (+0.88%) ⬆️
aiida/backends/djsite/globalsettings.py 86.84% <0%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e200fcd...378656b. Read the comment docs.

@DropD DropD requested a review from giovannipizzi June 1, 2018 09:03
@DropD DropD self-assigned this Jun 1, 2018
@DropD
Copy link
Contributor

DropD commented Jun 1, 2018

@giovannipizzi I don't see a problem with this, but I would like your approval.

@giovannipizzi
Copy link
Member

giovannipizzi commented Jun 1, 2018

Indeed it might be sufficient.

Could you also update the docstrings in the same file (for some reason the sentence stops...), and
https://github.com/espenfl/aiida_core/blob/e5b6ab8a25fbce5462be03f5a700d43675378832/aiida/orm/data/structure.py#L229

Great if you could also check the current tests for StructureData, and try to duplicate all of them to use X and check that everything still works

@espenfl
Copy link
Contributor Author

espenfl commented Jun 7, 2018

Most of the tests duplicated (including ASE test). Tests are successful.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Thanks!

@giovannipizzi giovannipizzi merged commit f7fe0a4 into aiidateam:develop Jun 7, 2018
@sphuber sphuber mentioned this pull request Jun 7, 2018
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