Correct typing of TaskDoc.run_stats, add number of electronic steps per ionic step to TaskDoc #1167
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1167 +/- ##
===========================================
+ Coverage 72.25% 90.32% +18.06%
===========================================
Files 77 147 +70
Lines 5136 14509 +9373
===========================================
+ Hits 3711 13105 +9394
+ Misses 1425 1404 -21 ☔ View full report in Codecov by Sentry. |
esoteric-ephemera
changed the title
esoteric-ephemera
force-pushed
the
run_stats
branch
from
c4a168c to
f24edd6
Compare
There was a problem hiding this comment.
looks great, thanks @esoteric-ephemera! 👍
only some minor suggestions and maybe a unit test for the new num_electronic_steps
emmet-core/emmet/core/tasks.py
Outdated
| @@ -434,7 +434,7 @@ class TaskDoc(StructureMetadata, extra="allow"): | |||
| description="Identifier for this calculation; should provide rough information about the calculation origin and purpose.", | |||
| ) | |||
|
|
|||
| run_stats: Optional[RunStatistics] = Field( | |||
| run_stats: Optional[dict[str, RunStatistics]] = Field( | |||
There was a problem hiding this comment.
type annotation uses dict instead of Mapping which is used elsewhere in the codebase for similar cases
There was a problem hiding this comment.
Changed to mapping, also added some basic tests to the TaskDoc tests - we don't have specific unit tests for IonicStep, would make sense to add these in a later PR if needed
| @model_validator(mode="after") | ||
| def set_elec_step_count(self): | ||
| if self.electronic_steps is not None and self.num_electronic_steps is None: | ||
| self.num_electronic_steps = len(self.electronic_steps) | ||
| return self |
There was a problem hiding this comment.
maybe handle the case where num_electronic_steps is set but doesn't match len(electronic_steps). probably len(electronic_steps) should take precedence or an error be raised if there's such a thing as a strict mode
There was a problem hiding this comment.
could also validate that num_electronic_steps is non-negative
There was a problem hiding this comment.
Thanks @janosh - think I would lean towards always overwriting the value of num_electronic_steps. We do the same now with run / task / calc type to ensure that they're always up to date with parsing changes
That should also take care of the negative number of steps
esoteric-ephemera
force-pushed
the
run_stats
branch
from
2d68ed2 to
18b8c58
Compare
…l ; fix molecule trajectory list indexing with optional args
…lve various mypy issues
…from gh actions ;
esoteric-ephemera
force-pushed
the
run_stats
branch
from
4499afd to
84d4b22
Compare
|
@tschaume OK if I merge? |
|
Yes, looks good, go ahead. We can do a release candidate to test the changes in the builders before a full release. |
Closes two issues:
TaskDoc.run_statsnum_electronic_stepstoemmet.core.vasp.calculation.{IonicStep, CalculationOutput}. The number of electronic steps in each ionic step is accessible viaTaskDoc.calcs_reversed[...].output.num_electronic_steps.OutputDoc#1035 by correcting the unit of force inemmet.core.tasks.OutputDocto eV/Å (this was just a typo, the output in VASP defaults to eV/Å for force units)Should also fix some pydantic-related issues