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

Add dir_name to ForceFieldMaker and **task_document_kwargs to from_ase_compatible_result #791

Merged
merged 8 commits into from
Apr 4, 2024

Conversation

QuantumChemist
Copy link
Contributor

I need to write the trajectory of a GAPrelax run into a file and then later access the directories of the files. That's why I added dir_name to ForceFieldRelaxMaker and **task_document_kwargs to ForceFieldTaskDocument.from_ase_compatible_result because as far as I can tell, that hasn't been implemented like that yet.

…ForceFieldTaskDocument.from_ase_compatible_result
@QuantumChemist QuantumChemist changed the title Add dir_name to ForceFieldMaker and **task_document_kwargs to rom_ase_compatible_result Add dir_name to ForceFieldMaker and **task_document_kwargs to from_ase_compatible_result Mar 28, 2024
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

perhaps related? materialsproject/jobflow#570

src/atomate2/forcefields/jobs.py Outdated Show resolved Hide resolved
@QuantumChemist
Copy link
Contributor Author

perhaps related? materialsproject/jobflow#570

Having access to job_dir would be something handy in general!
I went with dir_name like in other task documents.

@QuantumChemist
Copy link
Contributor Author

Just wanted to ask if this PR is ok like that, @janosh ? :)

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

apologies, didn't realize this was waiting on me. just 1 minor suggestions re testing

# run the flow or job and ensure that it finished running successfully
responses = run_locally(job, ensure_success=True)

assert Path(responses[job.uuid][1].output.dir_name).exists()
Copy link
Member

Choose a reason for hiding this comment

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

i think we can move this line to one of the existing tests and avoid having to run another 25 relax steps in CI to keep runtime low

@QuantumChemist
Copy link
Contributor Author

apologies, didn't realize this was waiting on me. just 1 minor suggestions re testing

@janosh no prob at all :) I adjusted the unit test now. I hope this is ok like that!

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

perfect, thank you!

auto-merge was automatically disabled April 3, 2024 19:31

Head branch was pushed to by a user without write access

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.91%. Comparing base (2687443) to head (04e0c01).
Report is 106 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #791      +/-   ##
==========================================
- Coverage   77.02%   76.91%   -0.12%     
==========================================
  Files         122      122              
  Lines        9106     9108       +2     
  Branches     1429     1429              
==========================================
- Hits         7014     7005       -9     
- Misses       1663     1675      +12     
+ Partials      429      428       -1     
Files Coverage Δ
src/atomate2/forcefields/jobs.py 92.91% <100.00%> (+0.11%) ⬆️
src/atomate2/forcefields/schemas.py 96.42% <ø> (ø)

... and 1 file with indirect coverage changes

@QuantumChemist
Copy link
Contributor Author

@janosh can you please merge it? :) auto merge was disabled and I had to fix merge conflicts with the main branch!

@janosh janosh merged commit c831245 into materialsproject:main Apr 4, 2024
7 checks passed
hrushikesh-s pushed a commit to hrushikesh-s/atomate2 that referenced this pull request Jun 28, 2024
…e_compatible_result (materialsproject#791)

* added dir_name to ForceFieldRelaxMaker and **task_document_kwargs to ForceFieldTaskDocument.from_ase_compatible_result

* Simplify taskdoc update

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>

* added docstring for task_doc_kwargs in force field schemas

* changed the unit test for the force_field_task_doc attributes

* fix linting

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
@utf utf added the enhancement Improvements to existing features label Aug 20, 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.

3 participants