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

fix bug with nmr set writing units #2509

Merged
merged 2 commits into from Apr 28, 2022

Conversation

jacksund
Copy link
Contributor

resolves #2508

@shyuep
Copy link
Member

shyuep commented Apr 27, 2022

Please add a Unittest to catch this bug and prevent it from recurring.

@jacksund
Copy link
Contributor Author

Yep, I'm working on that now 😄

@coveralls
Copy link

coveralls commented Apr 27, 2022

Coverage Status

Coverage decreased (-0.7%) to 83.279% when pulling 08daf15 on jacksund:fix-nmr-set into 6c84eb5 on materialsproject:master.

@jacksund
Copy link
Contributor Author

jacksund commented Apr 27, 2022

I was trying to add a lower-level check to all float-type INCAR settings, but am unsure the best place to do this. Maybe this would be in the get_string method of the Incar class? Might be something to consider in order to avoid issues with other vasp sets.

def get_string(self, sort_keys: bool = False, pretty: bool = False) -> str:

And the addition would be something like...

elif isinstance(self[k], float):
    # note the added float() which ensures FloatWithUnit conversion
    lines.append([k, " ".join([str(float(i)) for i in self[k]])])
# plus another check for a list of floats

EDIT: currently this PR only implements a check on the NMR set. This lower-level check would need to be a separate PR

@jacksund
Copy link
Contributor Author

@shyuep a unittest is in place and this is ready for review

@shyuep shyuep merged commit 508ed53 into materialsproject:master Apr 28, 2022
@shyuep
Copy link
Member

shyuep commented Apr 28, 2022

Thanks.

@jacksund jacksund deleted the fix-nmr-set branch June 22, 2022 13:19
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.

QUAD_EFG includes units for efg mode of MPNMRSet
3 participants