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 doc: adding metadata to flow #638

Merged
merged 10 commits into from
Jan 10, 2024
Merged

Conversation

naik-aakash
Copy link
Contributor

Addition

When doing a large number of calculations, it would be helpful to be able to add metadata to filter results from the database. Currently I did not find this information available in the atomate2 docs. (Maybe I am mistaken )

Thus I have added the solution provided here.

I think it would be nice to have this information readily available in the main atomate2 documentation itself.

@naik-aakash
Copy link
Contributor Author

Hi @janosh , I made this small addition to the documentation which I think should be useful for new users. So kindly review it and if you feel it's fine then this PR could be merged.

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (af667d8) 76.21% compared to head (89f610d) 76.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
- Coverage   76.21%   76.10%   -0.11%     
==========================================
  Files          86       87       +1     
  Lines        7109     7132      +23     
  Branches     1053     1055       +2     
==========================================
+ Hits         5418     5428      +10     
- Misses       1368     1381      +13     
  Partials      323      323              
Files Coverage Δ
src/atomate2/cp2k/powerups.py 88.88% <100.00%> (+2.22%) ⬆️
src/atomate2/vasp/powerups.py 93.54% <100.00%> (+1.54%) ⬆️
src/atomate2/common/powerups.py 81.81% <81.81%> (ø)

... and 1 file with indirect coverage changes

@naik-aakash
Copy link
Contributor Author

naik-aakash commented Jan 8, 2024

Hi @utf and @janosh , I have added powerup as suggested. Can you please review it and would be happy to address any changes if desired.

I have also added a powerup update_vasp_custodian_handlers to disable or use custom custodian handlers for vasp jobs. Could help in addressing the documentation for #639.

If this is okay, can also add an example for usage of this powerup as well.

Copy link
Member

@utf utf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @naik-aakash, a couple of comments.

src/atomate2/utils/powerups.py Outdated Show resolved Hide resolved
@@ -0,0 +1,68 @@
"""Utilities for modifying workflow."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move these to the src/atomate2/vasp/powerups.py directory and set the default class filter to be BaseVaspMaker.

Copy link
Contributor Author

@naik-aakash naik-aakash Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @utf , thinking a bit more about the location of this module, maybe is it not better to keep it more general?

Like it could also be used for adding metadata/modify custodian handlers ,other types of calculations for example Lobster runs, or cp2k, abinit etc in future?

Just wanted to know your 2nd opinion

Copy link
Member

@utf utf Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have a general implementation but then we should include a specific implementation for vasp/cp2k as well. I would also suggest that atomate2/common/powerups.py is a better place for the common implementation to live.

Then in atomate2/vasp/powerups.py you would do:

from atomate2.common.powerups import add_metadata_to_flow as base_add_metadata_to_flow

def add_metadata_to_flow(...)
    return base_add_metadata_to_flow(..., class_filter=BaseVaspMaker)

Obviously, you'll need to replace the ... and add the docstrings.

Copy link
Contributor Author

@naik-aakash naik-aakash Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @utf , I think am done with the changes suggested. Let me know anything else needs to be done to merge this PR.

Have also added small tests for these powerups

@utf
Copy link
Member

utf commented Jan 10, 2024

This looks awesome. Thank you so much!

@utf utf enabled auto-merge January 10, 2024 11:40
@naik-aakash
Copy link
Contributor Author

This looks awesome. Thank you so much!

You are most welcome 😃

@utf utf merged commit 4cd1e19 into materialsproject:main Jan 10, 2024
6 checks passed
@utf utf added the docs Improvements or additions to documentation label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants