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

Update lobster task schema #529

Merged
merged 47 commits into from
Nov 16, 2023

Conversation

naik-aakash
Copy link
Contributor

@naik-aakash naik-aakash commented Sep 22, 2023

Todo

  • reorder the lobstertask doc schema
  • include optional JSON data generation
  • update LobsterTaskDocument to have calc quality summary dict
  • add calc quality summary text to LobsterTaskDocument
  • make coop, cohp and cobicar storing in LobsterTaskDocument optional (can be very large)
  • add tests
  • Fix non gzip vasp files in lobster jobs ( Makes lobster schema fail : issue introduced after some refactoring in earlier PR 726a34f#diff-c29012eae961867ce5ed6ab6c32e0d63ef2de72e6830e7bec862dedcc5df76f7 )
  • make schema work for non gzip files as well ( at the moment it is hard-coded to work only with gzip files)
  • Add model for the dicts within CalcQualitySummary model
  • Add models for dicts nested down the schema,
  • add missing type hints/defaults

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #529 (202f559) into main (2092a36) will increase coverage by 0.21%.
The diff coverage is 91.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
+ Coverage   75.54%   75.76%   +0.21%     
==========================================
  Files          83       83              
  Lines        6808     6997     +189     
  Branches     1008     1043      +35     
==========================================
+ Hits         5143     5301     +158     
- Misses       1351     1375      +24     
- Partials      314      321       +7     
Files Coverage Δ
src/atomate2/lobster/files.py 91.66% <ø> (ø)
src/atomate2/lobster/schemas.py 91.13% <91.16%> (-1.62%) ⬇️

... and 2 files with indirect coverage changes

@naik-aakash naik-aakash changed the title [WIP] Update lobster task schema Update lobster task schema Sep 28, 2023
@naik-aakash
Copy link
Contributor Author

Hi @JaGeo , I think , I am done adding the desired changes, would be great, if you can have a look at it as well 😃 and would be happy to address if any more changes are needed

tests/vasp/lobster/schemas/test_lobster.py Outdated Show resolved Hide resolved
tests/vasp/lobster/flows/test_lobster.py Show resolved Hide resolved
src/atomate2/lobster/schemas.py Outdated Show resolved Hide resolved
src/atomate2/lobster/schemas.py Show resolved Hide resolved
@Zhuoying
Copy link
Contributor

image
Hi @naik-aakash Let me know when you fix this.

Thanks for reviewing this @JaGeo! I am helping to push the PRs moving forward as needed. When it is done, I will leave a comment here as ready-to-merge

@JaGeo
Copy link
Member

JaGeo commented Oct 10, 2023

@Zhuoying Thanks! Yes, we are still working on this. We were just releasing a new LobsterPy version today so hat we can make the final changes.

@Zhuoying
Copy link
Contributor

@JaGeo Sounds great! Thanks for letting me know. Ping me anytime when you think it is ready to merge.

@naik-aakash
Copy link
Contributor Author

Hi @JaGeo , I have now updated the lobsterpy version and tests also has been adapted accordingly. If you think, it is fine, I can ask for it to be merged 😃

@JaGeo
Copy link
Member

JaGeo commented Oct 10, 2023

@naik-aakash Yes, looks good!

@naik-aakash
Copy link
Contributor Author

Hi @JaGeo, I pushed the changes to make schema work even if gzipping is switched off. Can you have a look and let me know if this change is fine? (I do not expect you to look into it on the weekend 😉 ).

I have added a test as well, and it seems to work.

PS:
I tried using the existing atomate2 utils, but the code seems to get a bit messy. Thus, I came up with writing this FileNames class that seemed much simpler.

@naik-aakash
Copy link
Contributor Author

Hi @munrojm and @JaGeo, I am done addressing the comments. Let me know if anything more needs to be done in this PR.

@munrojm
Copy link
Member

munrojm commented Nov 8, 2023

Hi @munrojm and @JaGeo, I am done addressing the comments. Let me know if anything more needs to be done in this PR.

Things look good to me.

@naik-aakash naik-aakash changed the title [WIP] Update lobster task schema Update lobster task schema Nov 9, 2023
@naik-aakash
Copy link
Contributor Author

Hi @utf, I think this PR is ready to be merged if there are no more concerns related to code requirements.

@JaGeo
Copy link
Member

JaGeo commented Nov 15, 2023

Could someone review this as this is central to getting the Lobster data into the MP databas? 😃 Thanks in advance

@utf
Copy link
Member

utf commented Nov 16, 2023

Thanks for all your work on this. @naik-aakash @JaGeo

@utf utf merged commit 0738890 into materialsproject:main Nov 16, 2023
7 checks passed
@utf
Copy link
Member

utf commented Nov 16, 2023

As a separate thought, if lobster will be on the MP database, should we move these schemas to emmet?

@JaGeo
Copy link
Member

JaGeo commented Nov 16, 2023

I assume once everything works well, we probably need to do this.

(I would also like to implement a workflow compatible with the current workflow in atomate, i.e. same exchange correlation functional etc )

@utf utf added the enhancement Improvements to existing features label Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants