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

Electrode Workflow #655

Merged
merged 57 commits into from
Feb 29, 2024
Merged

Electrode Workflow #655

merged 57 commits into from
Feb 29, 2024

Conversation

jmmshn
Copy link
Contributor

@jmmshn jmmshn commented Dec 20, 2023

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:

  • tests

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: Patch coverage is 82.92683% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 76.39%. Comparing base (031521d) to head (f16c016).
Report is 1 commits behind head on main.

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     
Files Coverage Δ
src/atomate2/vasp/flows/electrode.py 90.00% <90.00%> (ø)
src/atomate2/cli/dev.py 0.00% <0.00%> (ø)
src/atomate2/common/flows/electrode.py 82.69% <82.69%> (ø)
src/atomate2/common/jobs/electrode.py 84.26% <84.26%> (ø)

update structure matcher

update structure matcher

update structure matcher

update structure matcher

update structure matcher
debugging

debugging

debugging

debugging

debugging

debugging

debugging

debugging

debugging

debugging

debugging

debugging

debugging

debugging
@utf
Copy link
Member

utf commented Jan 18, 2024

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?

@jmmshn
Copy link
Contributor Author

jmmshn commented Jan 18, 2024

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.

@jmmshn
Copy link
Contributor Author

jmmshn commented Jan 18, 2024

This discussion seems related:
materialsproject/jobflow#519

So the idea that the UUIDS should be sortable in time might be something to discuss.

@utf
Copy link
Member

utf commented Jan 18, 2024

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.

@jmmshn
Copy link
Contributor Author

jmmshn commented Jan 18, 2024

Thanks, @utf think something like this ca be added with UUID1:
Haveing this and having UUID1 be supported by the MPID class is (probably) all that I need to get the downstream stuff working.

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)

@utf utf mentioned this pull request Feb 15, 2024
13 tasks
@utf utf enabled auto-merge February 24, 2024 19:43
@utf
Copy link
Member

utf commented Feb 24, 2024

Hi @jmmshn, I've updated the emmet version and monty version. Looks like the remaining failing tests are related to this PR.

auto-merge was automatically disabled February 26, 2024 00:33

Head branch was pushed to by a user without write access

@jmmshn
Copy link
Contributor Author

jmmshn commented Feb 26, 2024

This should be fixed, the ulid import was missing.

@utf
Copy link
Member

utf commented Feb 26, 2024

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!

@utf
Copy link
Member

utf commented Feb 28, 2024

@jmmshn just this comment to address:

can you also add the ulid dependency to the "defects" class of optional dependencies

@jmmshn
Copy link
Contributor Author

jmmshn commented Feb 29, 2024

oops. Misses a save on the last commit.

@utf
Copy link
Member

utf commented Feb 29, 2024

Thanks @jmmshn!

@utf utf merged commit 041ef53 into materialsproject:main Feb 29, 2024
7 checks passed
@utf utf added the feature A new feature being added label Feb 29, 2024
@jmmshn
Copy link
Contributor Author

jmmshn commented Mar 1, 2024

@utf, ooops! I updated the pyproject.toml in the other open PR instead. Thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature being added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants