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

Changes default values for empty states and adds warnings for SPHInX #1567

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

skatnagallu
Copy link
Contributor

As a part of fixing the issues mentioned here, we thought it would be nice to have some warnings. Also we saw that there was discrepency between default empty states value and documentation of the function.

skatnagallu and others added 5 commits September 17, 2024 12:58
@skatnagallu
Copy link
Contributor Author

This is the previous PR

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10901773764

Details

  • 2 of 6 (33.33%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 70.88%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/sphinx/base.py 2 6 33.33%
Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/sphinx/base.py 1 79.75%
Totals Coverage Status
Change from base Build 10896531720: -0.02%
Covered Lines: 10778
Relevant Lines: 15206

💛 - Coveralls

@samwaseda
Copy link
Member

This is what I don’t understand. You’re right that the default may not have been properly chosen, but the new one appears also arbitrary to me. Can you maybe tell why you chose this value?

@skatnagallu
Copy link
Contributor Author

skatnagallu commented Sep 17, 2024

This is what I don’t understand. You’re right that the default may not have been properly chosen, but the new one appears also arbitrary to me. Can you maybe tell why you chose this value?

The default was already mentioned in the docstring of set_empty_states() function. I am not sure who originally implemented it. But the value I implemented is based on disscusion with @freyso and also what we saw in VASP, to match what was in the docstring.

@skatnagallu
Copy link
Contributor Author

@samwaseda Are you happy with this?

@samwaseda
Copy link
Member

ok I originally implemented it but I didn't realise that there was this discrepancy :D I'm still concerned about the warning, which will keep appearing to everyone, regardless of what kind of material we are talking about. I guess it's slightly unfair regarding other more serious warnings.

In addition, I don't quite understand the difference between the two print statements. Isn't one warning for the filling check enough?

@skatnagallu
Copy link
Contributor Author

For convergence there are two convergence checks so two print statements. We thought the print statement for scf convergence would be a helpful hint for new users of sphinx.

@samwaseda
Copy link
Member

For convergence there are two convergence checks so two print statements. We thought the print statement for scf convergence would be a helpful hint for new users of sphinx.

Sorry what I meant was scf_convergence is set with the nbands_convergence_check, so if the first print statement is given, then the second one is not required, right? Maybe I'm badly mistaken somewhere.

@skatnagallu
Copy link
Contributor Author

scf_convergence is set from parsing of the log file here. And it doesn't have information on the nbands_convergence_check. The final convergence_check has both informations in it.

@jan-janssen jan-janssen marked this pull request as draft December 17, 2024 15:35
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