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

Determine local environments based on bonding analysis #2057

Merged
merged 27 commits into from Feb 25, 2021
Merged

Determine local environments based on bonding analysis #2057

merged 27 commits into from Feb 25, 2021

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Feb 5, 2021

Summary

This is work in progress to add an analysis of the local environments based on bonding analysis. The classes will allow to both use localenv and chemenv in combination with Lobster output. It will also provide additional methods to analyse the ICOHPs/COHPs for sites with neighbor atoms.

@mkhorton , let me know if you already have comments.

I hope it is okay to put it in pymatgen.analysis.lobster_env

@shyuep
Copy link
Member

shyuep commented Feb 5, 2021

I would like to have a better organization of all the local environment analysis packages, including local_env and chemenv. Can we create a parent package called pymatgen.analysis.local_environment to house all of them. I would also prefer some common API that unifies all the different kinds of analysis and the various options. Right now, the packages are so big, fragmented and difficult to find and use.

@JaGeo
Copy link
Member Author

JaGeo commented Feb 5, 2021

I am happy to move my code to another place.

I am not sure regarding the common interface and moving the localenv and chemenv code (it will probably break a lot of existing code and documentation in publications and so on) but there are other people that might be able to comment better on this.

@shyuep
Copy link
Member

shyuep commented Feb 5, 2021

Well, all this is open for discussion. There is no need to create breaking changes. We can always retain existing code where they are with deprecation messages. @mkhorton what do you think?

@mkhorton
Copy link
Member

mkhorton commented Feb 5, 2021

The main problem is that chemenv and local_env are developed by different people/teams and have different philosophies.

The code @JaGeo is proposing is related to neither, but based on the Lobster code.

There has been some attempt at having a common interface, namely the NearNeighbors class (in local_env) is an interface to define neighbors and StructureGraph in pymatgen.analysis.graphs is a container to store the resulting graph. I think both of these are good.

@JaGeo here is using NearNeighbors here, so is conformant with that interface, so I think this is a good PR. I think it should be in its own file too, due to its length, but perhaps io.lobster is the better location since this is so closely coupled to the output of the Lobster code.

Chemenv (unrelated to this PR) pre-dates the NearNeighbors class, but it would be nice to get a NearNeighbors interface to Chemenv as well. It is too large a module to merge with local_env however, but a better page on all our local environment algorithms would be good in the docs to explain that we do provide multiple algorithms, and to give the relevant publications for each.

@JaGeo
Copy link
Member Author

JaGeo commented Feb 5, 2021

I am totally fine with moving the code to io.lobster.
Building some interface to ChemEnv via the NearNeighbors class also seems to be feasible (but I would expect that it would only be able to use a limited number of the capabilities of ChemEnv).

@mkhorton
Copy link
Member

mkhorton commented Feb 5, 2021

Building some interface to ChemEnv via the NearNeighbors class also seems to be feasible (but I would expect that it would only be able to use a limited number of the capabilities of ChemEnv).

Yes, perhaps it depends on what kwargs are available for ChemEnvNeighbors(NearNeighbors), I hope it may be possible to include the advanced functionality too. But ChemEnv is outside the scope of this PR so let's not worry about it here.

@coveralls
Copy link

coveralls commented Feb 23, 2021

Coverage Status

Coverage decreased (-0.4%) to 79.521% when pulling c13852b on JaGeo:master into 754c259 on materialsproject:master.

@JaGeo JaGeo changed the title [WIP] Determine local environments based on bonding analysis Determine local environments based on bonding analysis Feb 25, 2021
@JaGeo
Copy link
Member Author

JaGeo commented Feb 25, 2021

I am now quite happy with the current state of the code. I would be happy if this could be merged for now. (I will probably extend the code within the next months)
I am still having problems with some of the tests though and I am not completely sure what is going on. At least all tests on ubuntu work.

@shyuep shyuep merged commit 837013e into materialsproject:master Feb 25, 2021
@shyuep
Copy link
Member

shyuep commented Feb 25, 2021

I know what the issue with the mac/win tests is. I have made changes. I have merged this PR. Thanks.

@JaGeo
Copy link
Member Author

JaGeo commented Feb 26, 2021

Thank you.

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