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 Nernst-Einstein approximation #64

Merged
merged 13 commits into from
Mar 20, 2023
Merged

Added Nernst-Einstein approximation #64

merged 13 commits into from
Mar 20, 2023

Conversation

arthurfl
Copy link
Collaborator

@arthurfl arthurfl commented Dec 2, 2022

No description provided.

Copy link
Collaborator

@HakyungKwon-TRI HakyungKwon-TRI left a comment

Choose a reason for hiding this comment

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

Looks good, but can we make sure that the computed properties 1) are part of get_all_properties in analysis.py and 2) have associated tests?

@HakyungKwon-TRI
Copy link
Collaborator

Additionally, were you planning on adding other methods as well, or just the NE approx?

@arthurfl
Copy link
Collaborator Author

arthurfl commented Dec 6, 2022

Looks good, but can we make sure that the computed properties 1) are part of get_all_properties in analysis.py and 2) have associated tests?

Yes, will do this week!

@arthurfl
Copy link
Collaborator Author

arthurfl commented Dec 6, 2022

Additionally, were you planning on adding other methods as well, or just the NE approx?

Just the NE approximation - another route would be implementing the Einstein's relation, but that poses serious concerns regarding the convergence of the conductivity, especially with limited time series (~50 ns is typically far from enough for polymer electrolytes). The same would apply for the Green-Kubo relation.

Copy link
Contributor

@arashkhajeh-AMDD arashkhajeh-AMDD left a comment

Choose a reason for hiding this comment

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

LGTM

@arthurfl
Copy link
Collaborator Author

I've updated the test data with Nernst-Einstein quantities (conductivity and transference number), as well as get_all_properties().

By the way, I've absolutely failed installing both the current version (through pip/conda) and the one of this branch (through docker), is that stable on other boxes?

@HakyungKwon-TRI
Copy link
Collaborator

@arthurfl please resolve the conflicts and merge. thank you!

@arthurfl
Copy link
Collaborator Author

arthurfl commented Mar 1, 2023

Conflicts should be resolved now

@HakyungKwon-TRI
Copy link
Collaborator

@arthurfl it's failing some tests, one of which is @arashkhajeh-AMDD's test on population matrix?

Copy link
Contributor

@arashkhajeh-AMDD arashkhajeh-AMDD left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@arashkhajeh-AMDD arashkhajeh-AMDD left a comment

Choose a reason for hiding this comment

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

Great. Thanks!

@HakyungKwon-TRI HakyungKwon-TRI merged commit 4076e9b into main Mar 20, 2023
@HakyungKwon-TRI HakyungKwon-TRI deleted the nernst-einstein branch March 20, 2023 18:08
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.

3 participants