PDOSWorkChain - align energy range to fermi level #764
Conversation
| @@ -219,14 +222,15 @@ def define(cls, spec): | |||
| help='Terminate workchain steps before submitting calculations (test purposes only).' | |||
| ) | |||
| spec.input( | |||
| 'align_to_fermi', | |||
| valid_type=orm.Bool, | |||
| 'fermi_energy_range', | |||
There was a problem hiding this comment.
I might be saying something stupid because I wasn't part of the discussions, but does the name fermi_energy_range really make sense? The values define the energy window in which the DOS is computed. Does something like just energy_range or energy_window not make more sense?
There was a problem hiding this comment.
I see your point. If a energy window is specified by this parameter, the DOS/PDOS is computed in this range with respect to the fermi energy. For example, the range 0 - 30 eV is specified and the fermi energy is 7 eV, then the DOS would be computed in the range 7 - 37 eV. Therefore, I think it would be useful to indicate this also by the name of the input parameter (but maybe the description in the help keyword would be already enough).
There was a problem hiding this comment.
To me it reads a bit weird having it in the option, so I would vote to put it simply in the description that it is relative to the fermi level. I will let @mbercx have the final vote.
There was a problem hiding this comment.
Sorry for being later to the party! Although I agree the name isn't perfect, I still think it's better to highlight that it's an energy range around the Fermi level by adding it to the name. Maybe energy_range_fermi would give less of an impression that there is some range of Fermi levels considered?
EDIT: Or energy_range_vs_fermi, if that isn't too long?
There was a problem hiding this comment.
Personally, I like the last one but I leave this up to you
There was a problem hiding this comment.
I also vote for the latter 👍 A few more characters is almost always preferable if it adds clarity. So @t-reents please feel free to go with that one, update the PR and we can merge. Thanks!
| @@ -219,14 +222,15 @@ def define(cls, spec): | |||
| help='Terminate workchain steps before submitting calculations (test purposes only).' | |||
| ) | |||
| spec.input( | |||
| 'align_to_fermi', | |||
| valid_type=orm.Bool, | |||
| 'fermi_energy_range', | |||
There was a problem hiding this comment.
To me it reads a bit weird having it in the option, so I would vote to put it simply in the description that it is relative to the fermi level. I will let @mbercx have the final vote.
|
Thanks again @sphuber. I committed your suggestions. |
|
Thanks a lot for your comments and instructions! @sphuber |
|
The tests are failing. Apparently the validator is being called even if no value is specified. We could solve this by explicitly checking for this value in the validator and in that case just returning, but really this should not be happening. I am looking into |
mbercx
force-pushed
the
issue_716_PDOS_WorkChain_align_energy_range_to_fermi_level
branch
from
3f22610
to
ba9716e
Compare
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
mbercx
force-pushed
the
issue_716_PDOS_WorkChain_align_energy_range_to_fermi_level
branch
from
ba9716e
to
a88dfc7
Compare
|
Tests pass, but maybe we should add some more to properly test the functionality with and without the Also: maybe we should move the long module docstring into the documentation page of the work chain? Right now it's a bit empty: https://aiida-quantumespresso--764.org.readthedocs.build/en/764/howto/workflows/pdos.html |
For sure, I will add the testst, soon. |
Hi,
I implemented the changes that where suggested in this comment #716 (comment).
I already talked about this PR with @mbercx. I am not familiar with pytest, so I did not include additional tests. I would be happy to get some help concerning the tests.