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

Switch to emmet's MoleculeMetadata #301

Merged
merged 32 commits into from
May 2, 2023
Merged

Switch to emmet's MoleculeMetadata #301

merged 32 commits into from
May 2, 2023

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Apr 8, 2023

Addresses #300, wherein I realized we had a MoleculeMetadata schema in both Atomate2 and emmet, resulting in near-duplicate blocks of code. I merged the two together in emmet here and updated Atomate2 to use emmet's MoleculeMetadata instead just like you did for StructureData. This should consolidate things within Atomate2 nicely. It's also coincidentally in line with Alex's recent efforts to move some more general schema things to emmet.

Edit: We get about 250 lines out of Atomate2 and into emmet (where they belong), which I consider a win.

@Andrew-S-Rosen Andrew-S-Rosen changed the title [WIP] Switch to emmet's MoleculeMetadata Switch to emmet's MoleculeMetadata Apr 8, 2023
@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Apr 9, 2023

@utf: This should be ready to merge. If you wouldn't mind bumping the version after this merge sometime, that'd be a great help!

@nwinner: You use the MoleculeMetadata in the cp2k schema. Your tests still pass with this PR, but one comment. You have include_structure and include_molecule in your cp2k task doc here. I think these are outdated. The former was removed from StructureMetadata when @utf ported things over to emmet. The latter I removed from MoleculeMetadata when porting things over to emmet for consistency. Assuming that these key/value pairs are doing nothing, you may wish to consider removing them in a separate PR.

@codecov
Copy link

codecov bot commented Apr 9, 2023

Codecov Report

Merging #301 (700aaf9) into main (5eae562) will decrease coverage by 0.36%.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
- Coverage   65.93%   65.57%   -0.36%     
==========================================
  Files          73       72       -1     
  Lines        7051     7018      -33     
  Branches      899      896       -3     
==========================================
- Hits         4649     4602      -47     
- Misses       2137     2150      +13     
- Partials      265      266       +1     
Impacted Files Coverage Δ
src/atomate2/common/schemas/cclib.py 74.00% <54.54%> (-1.35%) ⬇️
src/atomate2/common/jobs/__init__.py 100.00% <100.00%> (ø)
src/atomate2/cp2k/schemas/task.py 82.80% <100.00%> (-0.08%) ⬇️
src/atomate2/vasp/flows/lobster.py 95.55% <100.00%> (ø)

... and 1 file with indirect coverage changes

@Andrew-S-Rosen
Copy link
Member Author

@utf, could you give this a look when you get the chance? Thanks!

@utf
Copy link
Member

utf commented May 2, 2023

This looks great. Thanks Andrew

@utf
Copy link
Member

utf commented May 2, 2023

Ah there are some unresolved conflicts apparently.

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented May 2, 2023

Taken care of now, boss. Feel free to merge at your discretion!

@utf
Copy link
Member

utf commented May 2, 2023

Thanks!

@utf utf merged commit f365b37 into materialsproject:main May 2, 2023
6 checks passed
@Andrew-S-Rosen Andrew-S-Rosen deleted the molecule branch May 2, 2023 22:08
@utf utf added the enhancement Improvements to existing features label Sep 1, 2023
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.

None yet

2 participants