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 32 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?

@jonathanjdenney
Copy link
Contributor Author

@janosh
Sorry, I didn't realize it thinks I changed/added all those files. The ones you highlighted are the only ones I deliberately added. I'm guessing the others are the remnants of some merge error or something. I will get rid of them.

@jonathanjdenney
Copy link
Contributor Author

jonathanjdenney commented Sep 8, 2023

@janosh Sorry, I didn't realize it thinks I changed/added all those files. The ones you highlighted are the only ones I deliberately added. I'm guessing the others are the remnants of some merge error or something. I will get rid of them.

I'm looking through the extra files and in all of them they are treated as changed files but the addition and deletion lines are identical. I don't what happened, but the files are supposed to be there as they are in the main pymatgen code as well. I'm not quite sure how to move forward. Should I just delete those files from my local version and make that as a pull request? I feel like that would have the same problem of showing as 30 edited files.

@janosh
Copy link
Member

janosh commented Sep 8, 2023

I think you need to run sth like

git checkout master -- space/separated paths/to/files you/want/to/revert

@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?

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.

None yet

2 participants