Emit error when trying to set negative number of empty bands in DFT #1542
Conversation
|
Sphinx raised an error before, but I changed it to a warning. Vasp previously allowed to set negative numbers which could lead to negative NBANDS, which then of course crashed Vasp. In both cases I bump values <= 0 to 1, but we can discuss other defaults. |
|
@samwaseda I see the sphinx unit tests check for the for the ValueError. What do you think, is it safer to raise the error or is setting "reasonable" value ok? I have no strong feelings either way. |
|
That doesn’t sound reasonable to me. For example, it makes sense for me to set a random LAMMPS potential: “you didn’t set a potential, so we set one for you”. In this case, on the other hand, what this PR is basically saying is “you did set a value, but we change it”. Or can you tell me what would be a reasonable situation for this? |
|
I basically had I bug in my code that called |
for more information, see https://pre-commit.ci
pmrv
changed the title
pyiron_atomistics/sphinx/base.py
Outdated
| @@ -1075,8 +1075,10 @@ def set_empty_states(self, n_empty_states=None): | |||
| # will be converted later; see load_default_groups | |||
| self.input["EmptyStates"] = "auto" | |||
| else: | |||
| if n_empty_states < 0: | |||
| raise ValueError("Number of empty states must be greater than 0") | |||
| if n_empty_states <= 0: | |||
There was a problem hiding this comment.
| if n_empty_states <= 0: | |
| if n_empty_states < 0: |
0 should be ok
pyiron_atomistics/vasp/base.py
Outdated
| @@ -1333,6 +1333,10 @@ def set_empty_states(self, n_empty_states=None): | |||
| """ | |||
| n_elect = self.get_nelect() | |||
| if n_empty_states is not None: | |||
| if n_empty_states <= 0: | |||
There was a problem hiding this comment.
| if n_empty_states <= 0: | |
| if n_empty_states < 0: |
0 should be ok
No description provided.