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

Inaccuracy in RDF calculation over several structures #28

Open
vishankkumar opened this issue Sep 18, 2019 · 11 comments
Open

Inaccuracy in RDF calculation over several structures #28

vishankkumar opened this issue Sep 18, 2019 · 11 comments

Comments

@vishankkumar
Copy link

Dear Developers,

I have used RadialDistributionFunction class in vanhove module of this package and found one ambiguity. Input data are 5 structures with same composition.

While passing single structure, each of them have the first RDF peak at around 2 Angstrom but when I pass all the five structures, the resulting RDF (single plot) shows a peak at 2 Angstrom along with another peak below 2 Angstrom, which to me seems ambiguous. Will you please have a look at it and let me know.

Feel free to ask me if you need the input files for testing.

Thanks for your help in advance,
Regards,
Vishank

@shyuep
Copy link
Contributor

shyuep commented Sep 18, 2019

Can you send us the example structures?

@vishankkumar
Copy link
Author

vishankkumar commented Sep 18, 2019 via email

@shyuep
Copy link
Contributor

shyuep commented Sep 18, 2019

I don't see te file attached.

@vishankkumar
Copy link
Author

vishankkumar commented Sep 18, 2019 via email

@vishankkumar
Copy link
Author

Here are the files. Sorry, earlier I was trying to attach by email.
pymatgen_diffusion_rdf_test.tar.gz

@shyuep
Copy link
Contributor

shyuep commented Sep 18, 2019

Thanks. @HanmeiTang Pls look into the said issue.

@HanmeiTang
Copy link
Contributor

@shyuep Sure!

HanmeiTang added a commit that referenced this issue Sep 26, 2019
HanmeiTang added a commit that referenced this issue Sep 26, 2019
@HanmeiTang HanmeiTang reopened this Sep 26, 2019
HanmeiTang added a commit that referenced this issue Sep 26, 2019
@HanmeiTang
Copy link
Contributor

@shyuep @vishankkumar This issue should be solved.

The wrong result reported by @vishankkumar is caused by passing a list of structures with different lattices. In our original RadialDistributionFunction class, it assumes all structures are in the same lattice. Now, this bug has been fixed along with corresponding unittests.

The circleCi failed tests are coming from pymatgen_diffusion/neb/tests/test_full_path_mapper.py, which is irrelevant to this issue. @shyuep how about closing this solved issue and opening a new one to fix the failed tests, which may also need @jmmshn's assistance.

@jmmshn
Copy link
Contributor

jmmshn commented Sep 26, 2019

ooops I mixed up some code in the last PR will fix this

@vishankkumar
Copy link
Author

Thank you for solving the issue and I did not know that the lattice have to be same. I will test it and will let you know. Thanks.

@thienbinh92
Copy link

Hi, I found some features are non-exhaustive. Are they still problematic?

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

No branches or pull requests

5 participants