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

Make MigrationHop MSONable again #312

Merged
merged 10 commits into from
Jul 21, 2022
Merged

Conversation

acrutt
Copy link
Contributor

@acrutt acrutt commented Jul 20, 2022

Taking over a previous pull request that was abandoned: #264

@shyuep
Copy link
Contributor

shyuep commented Jul 20, 2022

Pls fix the test errors. The linting errors can be dealt with later, but the unittest must pass.

@shyuep
Copy link
Contributor

shyuep commented Jul 20, 2022

Pls merge the latest master. A lot of the problems will disappear.

@acrutt
Copy link
Contributor Author

acrutt commented Jul 20, 2022

@shyuep the test I added (pymatgen/analysis/diffusion/tests/test_pathfinder.py::PathfinderTest::test_mhop_msonable) now passes. Are any other changes needed?

@shyuep
Copy link
Contributor

shyuep commented Jul 20, 2022

The tests fail because there are changes in your PR with host_symm_structure. These need to be fixed.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2713423963

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 136 unchanged lines in 6 files lost coverage.
  • Overall coverage remained the same at 81.363%

Files with Coverage Reduction New Missed Lines %
pymatgen/analysis/diffusion/utils/edge_data_from_sc.py 4 94.81%
pymatgen/analysis/diffusion/utils/supercells.py 4 90.38%
pymatgen/analysis/diffusion/neb/pathfinder.py 10 96.18%
pymatgen/analysis/diffusion/aimd/rdf.py 16 81.91%
pymatgen/analysis/diffusion/neb/full_path_mapper.py 47 81.01%
pymatgen/analysis/diffusion/analyzer.py 55 61.42%
Totals Coverage Status
Change from base Build 2708090438: 0.0%
Covered Lines: 1659
Relevant Lines: 2039

💛 - Coveralls

@shyuep shyuep merged commit 60f9e46 into materialsvirtuallab:master Jul 21, 2022
@acrutt acrutt deleted the hop_patch branch July 21, 2022 17:44
@acrutt
Copy link
Contributor Author

acrutt commented Jul 21, 2022

@shyuep would it be possible to release an updated version of pymatgen-analysis-diffusion with this change on PyPi? I am working on code in emmet that depends on this patch.

@shyuep
Copy link
Contributor

shyuep commented Jul 21, 2022

Done.

@acrutt
Copy link
Contributor Author

acrutt commented Jul 21, 2022

Great! Thank you so much @shyuep for your responsiveness in getting this fix addressed in a timely manner.

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

3 participants