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

fix another CombinedData bug #2160

Merged
merged 8 commits into from Jun 9, 2021
Merged

Conversation

htz1992213
Copy link
Contributor

@htz1992213 htz1992213 commented Jun 2, 2021

Summary

Include a summary of major changes in bullet points:

  • Fix 1: The CombinedData constructor previously creates new ff dict only according to the keys in the first mol in the mols llist, which may cause the ff information in the remaining mols lost. The new implementation create the ff dict using a set of keys containing all ff keys in mols
  • Fix 2: The CombinedData constructor previously traverses the topo and ff info of mol in mols list without checking None type, which may cause error. The new implementation checks for None before iteration, and set ff/topo of the CombinedData to be None if all mol in mols list has None ff/topo.

Tests have been created accordingly.

@coveralls
Copy link

coveralls commented Jun 2, 2021

Coverage Status

Coverage decreased (-0.6%) to 83.082% when pulling 8a30eab on htz1992213:master into c724e7b on materialsproject:master.

@mkhorton
Copy link
Member

mkhorton commented Jun 2, 2021

Thanks @htz1992213. Rather than edit the pylintrc with this specific case, I think you can add an in-line comment (something like # ignore).

Can you comment on what the issue was with ComputedStructureEntry._composition? This is a protected member anyway, so shouldn't be access outside the class.

@htz1992213
Copy link
Contributor Author

Thanks for the advice @mkhorton! The access comes from the following line, I am not familiar with these classes so I will change the code with the # ignore workaround.

entry._composition /= factor

@rkingsbury
Copy link
Contributor

Thanks @htz1992213. Rather than edit the pylintrc with this specific case, I think you can add an in-line comment (something like # ignore).

Can you comment on what the issue was with ComputedStructureEntry._composition? This is a protected member anyway, so shouldn't be access outside the class.

@mkhorton I will note that ComputedStructureEntry._composition seems to be causing a 1pylint failure in another PR as well, in which the changes are completely unrelated. Does this test pass in master?
https://github.com/materialsproject/pymatgen/pull/2135/checks?check_run_id=2754058779

@htz1992213
Copy link
Contributor Author

Hey @mkhorton, could you please let me know if there are other things that I should address for the PR to be merged? Thanks!

@mkhorton
Copy link
Member

mkhorton commented Jun 9, 2021

This is all good, thank you Tingzheng!

@mkhorton mkhorton merged commit 77c6dd6 into materialsproject:master Jun 9, 2021
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.

None yet

4 participants