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

add completed_at field (+run_stats) to TaskDoc as root fields #1116

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

tsmathis
Copy link
Collaborator

@tsmathis tsmathis commented Sep 25, 2024

@esoteric-ephemera, this should be enough to get completed_at in as a root level field, right?

There were already kwargs of completed_at=calcs_reversed[0].completed_at, in the .from_structure calls, but the field isn't showing up currently

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.84%. Comparing base (de70eb1) to head (02838e4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1116   +/-   ##
=======================================
  Coverage   88.83%   88.84%           
=======================================
  Files         114      114           
  Lines       10627    10629    +2     
=======================================
+ Hits         9441     9443    +2     
  Misses       1186     1186           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@esoteric-ephemera
Copy link
Collaborator

@tsmathis that should be good - also worth noting that run_stats and include_structure are two other optional fields that are automatically populated by TaskDoc.{from_directory, from_vasprun}. We should probably make these default top-levels too right?

Also tweaked one of the builder tests to better account for when no POTCAR library is preset (like CI)

@tsmathis
Copy link
Collaborator Author

I can't really find anywhere that include_structure actually has an effect?

And the run_stats would be okay to include I guess since they were in TaskDocument. Will be just another field that we'll have to drop when parsing though.

There is also differing behavior for from_directory and from_vasprun for the run stats field, should they be the same?

@esoteric-ephemera
Copy link
Collaborator

I'm not sure what the purpose of include_structure is - it's not used anywhere in emmet or atomate2 and it's just a bool. I'd be tempted to change it to a property of TaskDoc that returns whether TaskDoc.structure is None

from_directory and from_vasprun have slightly different outputs - the from_vasprun method doesn't populate as many fields because it doesn't have access to things like volumetric objects. Their signatures are the same tho

@esoteric-ephemera
Copy link
Collaborator

Hey @utf, do you recall what the purpose of TaskDoc.include_structure is? We'd like to remove it from the TaskDoc.from_directory and TaskDoc.from_vasprun methods, since it's not defined in the base schema of TaskDoc.

@tsmathis tsmathis changed the title add completed_at field to TaskDoc add completed_at field (+run_stats) to TaskDoc as root fields Sep 26, 2024
@utf
Copy link
Member

utf commented Sep 26, 2024

IIRC it was an option as to whether the structure was stored alongside the metadata. It was originally part of StuctureMetadata.from_structure. I don't remember it being a field though.

@tsmathis
Copy link
Collaborator Author

tsmathis commented Sep 26, 2024

That was the impression we got based on the name, but we haven't found any place in either emmet or atomate2 where include_structure has an effect for TaskDoc.

Is this a behavior you'd like to see functioning for TaskDoc?

@utf
Copy link
Member

utf commented Sep 26, 2024

As long as the structure is always stored in the task doc that's fine. Sounds like it's safe to remove.

@tsmathis
Copy link
Collaborator Author

Okay, sounds good @utf, thanks for the input.

@esoteric-ephemera, lets follow up separately on the include_structure behavior and make sure no regressions are introduced.

That aside, going to merge this in unless there is anything else you'd like to see in these current changes?

@tsmathis tsmathis merged commit 8e0879c into main Sep 26, 2024
6 of 8 checks passed
@tsmathis tsmathis deleted the taskdoc-missing-fields branch September 26, 2024 20:14
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.

4 participants