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

Add F* diagram generator to pymatgen #3277

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

jonathanjdenney
Copy link
Contributor

@jonathanjdenney jonathanjdenney commented Aug 28, 2023

I have written code that takes a list of symmetrized structure objects and generates an f* diagram automatically with the plotly library.

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

Thanks @jonathanjdenney!

Can you please check if some of the test files we already have can be used in place of new ones and for the remaining new files, please gzip them.

@jonathanjdenney
Copy link
Contributor Author

@janosh
Everything on this pull request looks good from my end. Let me know if there are any changes you want me to make.

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

As I mentioned earlier, please try to minimize the number of new test files and compress the remaining.

@janosh janosh added enhancement A new feature or improvement to an existing one analysis Concerning pymatgen.analysis needs file dedup Needs file compression/deduplication, usually for test files labels Sep 2, 2023
@jonathanjdenney
Copy link
Contributor Author

@janosh
I got the tests to work with just one cif that is already in the test files directory. Please let me know if there are other changes you would like.

@janosh
Copy link
Member

janosh commented Sep 8, 2023

@jonathanjdenney There are still 33 changed files and 23k changed lines which make this PR very hard to review. The purpose of these 3 files is obvious.

Screenshot 2023-09-08 at 9 04 16 PM

Could you either explain the purpose of or remove the other 30 files?

@janosh
Copy link
Member

janosh commented Sep 16, 2023

@jonathanjdenney I removed all the excess files and gzipped pymatgen/analysis/fstar/neutron_factors.csv. The tests look a bit meagre. Maybe you can think of more things to test? E.g. uncovered edge cases?

@janosh janosh added needs testing PRs that are not ready to merge due to lacking tests and removed needs file dedup Needs file compression/deduplication, usually for test files labels Sep 16, 2023
jonathanjdenney and others added 8 commits September 18, 2023 09:45
Raise error is structures with less than 3 unique sites are used as an input.
Open neutron_factors.csv with gzip library
Raise an error if someone wants neutron scattering past atomic number 96.
@jonathanjdenney
Copy link
Contributor Author

@jonathanjdenney I removed all the excess files and gzipped pymatgen/analysis/fstar/neutron_factors.csv. The tests look a bit meagre. Maybe you can think of more things to test? E.g. uncovered edge cases?

I added a few ValueError messages for situations I know won't work. Specifically structures with less than 3 unique sites, and neutron scattering for atomic numbers greater than 96. I'm having trouble thinking of any more edge cases than that. I'm not sure of any specific tests I can add.

Also I would like to set up a tutorial for this, as some of it's features are not all that intuitive. What would be the best way to go about setting that up?

@janosh janosh force-pushed the master branch 2 times, most recently from 3c23114 to 36e289c Compare December 19, 2023 02:10
@janosh janosh force-pushed the master branch 4 times, most recently from d725325 to dca98be Compare February 2, 2024 11:47
@janosh janosh force-pushed the master branch 2 times, most recently from e3fbc67 to 41e6d99 Compare August 3, 2024 19:01
@shyuep
Copy link
Member

shyuep commented Nov 13, 2024

Thanks. One thing I would like you to do is to instead of a pymatgen/analysis/fstar/fstar.py structure, choose either pymatgen/analysis/fstar.py or pymatgen/analysis/fstar/__init__.py. Right now the import requires an additional nesting, which is unnnecessary. I prefer the former unless you believe there will be many more modules to be implemented within fstar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Concerning pymatgen.analysis enhancement A new feature or improvement to an existing one needs testing PRs that are not ready to merge due to lacking tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants