-
Notifications
You must be signed in to change notification settings - Fork 96
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
Electrode Workflow #655
Electrode Workflow #655
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #655 +/- ##
==========================================
+ Coverage 76.22% 76.39% +0.17%
==========================================
Files 88 91 +3
Lines 7191 7353 +162
Branches 1051 1070 +19
==========================================
+ Hits 5481 5617 +136
- Misses 1393 1410 +17
- Partials 317 326 +9
|
update structure matcher update structure matcher update structure matcher update structure matcher update structure matcher
The idea with jobflow/atomate2 is that we'd be moving away from using sequential mp_ids as these cause an issue when merging two databases. I'd prefer to stick with UUIDs in atomate2/jobflow, and leave the mp_id generation up to the MP ingestion pipeline. It seems like we're running into an issue of the scope of emmet. Is it solely the build pipeline for MP or is it also a definition of general schemas for materials chemistry. Ideally, the emmet models could function without mp_ids present. E.g., could we do some other sorting of the UUID if it doesn't match the mp format? |
Would it be possible to use UUID1 instead of UUID4 that way there is a time component encoded in the data. |
This discussion seems related: So the idea that the UUIDS should be sortable in time might be something to discuss. |
Switching to a sortable UUID with timestamp would be a good fix IMO. We can continue the discussion on the jobflow repo but I'm happy to get this pushed through quickly so it doesn't cause any issues with your workflows. |
Thanks, @utf think something like this ca be added with UUID1: import datetime
import uuid
uuid_arg = uuid.uuid1()
DATETIME_EPOC = datetime.datetime(1970, 1, 1)
timestamp = (uuid_arg.time - 0x01B21DD213814000) / 1e7
dt = DATETIME_EPOC + datetime.timedelta(seconds=timestamp)
print(dt) |
Hi @jmmshn, I've updated the emmet version and monty version. Looks like the remaining failing tests are related to this PR. |
Head branch was pushed to by a user without write access
This should be fixed, the |
Thanks @jmmshn, can you also add the ulid dependency to the "defects" class of optional dependencies and also resolve any outstanding conflicts. Then I can merge! |
@jmmshn just this comment to address:
|
oops. Misses a save on the last commit. |
Thanks @jmmshn! |
@utf, ooops! I updated the pyproject.toml in the other open PR instead. Thanks for fixing! |
Electrode Workflow
Added a general electrode insertion workflow.
Replicated the capabilities from the following wf from atomate1:
https://github.com/hackingmaterials/atomate/blob/6327b07ea1ae85fbbe29772777a80dfe4a94dc7b/atomate/vasp/workflows/base/electrode.py#L20
Minor change to dev script
Now allows users to specify additional files to copy
--additional_file CHGCAR --additional_file LOCPOT
TODO: