-
Notifications
You must be signed in to change notification settings - Fork 863
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
JDFTx IO module #4078
base: master
Are you sure you want to change the base?
JDFTx IO module #4078
Conversation
… top of __getattr__ magic method so it is more clear to the user what variables are accessible from a jdftxoutfile object (and other downstream class objects)
…done 100% to work properly without circular imports, but the size of this repo causes refactor symbol movement to take literally minutes, so keeping the structure like so for now
…stuff + JDFTXInfile item setting safety nets, additional testing for infile, and more informative README
Problems on readme line breaks Signed-off-by: benrich37 <[email protected]>
Problems on README Signed-off-by: benrich37 <[email protected]>
Problems on README Signed-off-by: benrich37 <[email protected]>
Problems on README Signed-off-by: benrich37 <[email protected]>
Signed-off-by: benrich37 <[email protected]>
Signed-off-by: benrich37 <[email protected]>
Updated README for JDFTX io branch Signed-off-by: benrich37 <[email protected]>
…emoving empty test modules
Signed-off-by: benrich37 <[email protected]>
Added pyproject.toml to workflow dependencies Signed-off-by: benrich37 <[email protected]>
…ymatgen fork as a dependency (anticipating the class objects not being kept in individual submodules for very long)
Adding imports to jdftx module's __init__ so I can start using this p…
@benrich37 thanks for this PR and for your patience! Is the current branch up-to-date, and are there are specific areas of concern? |
…ences" method in pmg Element class
Periodic table edits
Jeiters rename
Hey! Thank you for your comments! Sorry I hadn't checked this PR in a few days. My main concern for this PR is its readability - I wrote the JDFTx output's data structure to be object oriented to help develop faster, but I can understand how the hierarchy of objects can make it difficult to understand what's going on. I feel confident with functionality itself though (aside from edge cases for JDFTx outputs that keep popping up) |
From my side, can you explain the changes to periodic_table.py? Since it's a core class the changes should ideally be separated out from new functionality. |
There is a failing test that I am pretty sure is related to the changes to periodic table. |
Hi @shyuep ! Sorry yes - I thought it would be cleaner to have the Species valence(s) property defer to the Element valence(s) property, but I'm seeing now these need to be defined separately so that Species can incorporate oxidation state. Fixing right now. Also @computron - that makes to me, I wasn't sure if it would be better to have internal redundancy to the periodic_table module in this PR or to update alongside this IO module. @mkhorton - would it be okay to go back to using the ad-hoc method in the io.jdftx.data module for the time being? Edit: @computron sorry misread your comment - part of the JDFTx output parsing requires a method which can fetch the number of valence electrons for each element in a Structure. The original method that did this was redundant to data that could be fetched by periodic_table, but the valence property needed to do this would throw errors for mixed valence species. I'm okay with either a) Species and Element updated to include a valences property, or b) have my own method (for now) to get the number of valence electrons that works for mixed valence species. As long as there is consensus, I can roll back changes to periodic_table and re-introduce them in a new PR after this one is accepted. |
Fix for Species class's valence(s) property
… conflict with unexpected minimization keywords
Quick update: some of the dataclasses are having issues calling attributes that used to work. Should be fixed soon. Edit: Fixed now. |
JDFTXOutfileSlice __getattr__ fix
JMinSettings edge case fix
Definitely let's try to minimize internal redundancy. That kind of thing can get out of control quickly and lead us towards a messy codebase. I'll take another look at this soon, but if you want to arrange a quick call (~1hr) to review this PR I'm also open to that. I do not expect big issues but it might make it easier/quicker for us to get through it together. |
Summary
Major changes:
Changes to existing code:
core.periodic_table
(
if len(valence) > 1: raise ValueError(f"{self} has ambiguous valence")
)and return signature changed from
tuple[int | np.nan, int]
tolist[tuple[int | np.nan, int]]
(return valence
instead ofreturn valence[0]
)len(self.valences) > 1
, and otherwise returnself.valences[0]
(I have an idea of how to remove the redundancy between Species and ElementBase for valence, but I will hold off on this until the next PR)
Todos