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.

Sign up for GitHub

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

Jump to bottom

Validators and Jobs for Lobster #152

Merged
merged 9 commits into from
Apr 27, 2020
Merged

Validators and Jobs for Lobster #152

merged 9 commits into from
Apr 27, 2020

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Apr 27, 2020

Hi!

We (@gpetretto +I) are working on integrating Lobster workflows into atomate. Therefore, we would like to include new validators and jobs for Lobster into custodian.

We have also written tests. Please let me know if we should do any changes.

Best,
JG

shyuep
shyuep reviewed Apr 27, 2020
View reviewed changes
Copy link
Member

@shyuep shyuep left a comment

Choose a reason for hiding this comment

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

Other than the comments above, pls make sure you only have the necessary files in the test files. E.g., whether WAVECAR is necessary at all. If it is, that is fine. But we do not want to be introducing large binary files for no reason.

custodian/lobster/__init__.py Outdated
from __future__ import unicode_literals

"""
This package implements various VASP Jobs and Error Handlers.
Copy link
Member

Choose a reason for hiding this comment

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

Change this doc to say lobster?

Copy link
Member Author

Choose a reason for hiding this comment

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

okay

custodian/lobster/handlers.py
@@ -0,0 +1,82 @@
import os
Copy link
Member

Choose a reason for hiding this comment

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

There needs to be a module doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay

custodian/lobster/jobs.py
@@ -0,0 +1,81 @@
import logging
Copy link
Member

Choose a reason for hiding this comment

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

Module doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay

test_files/lobster/vasp_lobster_output/FW_submit.script Outdated
@@ -0,0 +1,20 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file should be in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove this.

test_files/lobster/vasp_lobster_output/FW.json Outdated
@@ -0,0 +1,136 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is this file even necessary. Custodian stands separate from Fireworks.

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove this as well.

test_files/lobster/lobsterouts/lobsterout.crash Outdated
@@ -0,0 +1,42 @@
LOBSTER v3.2.0 (g++ 5.4.0)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what this file is. Or the .keep file.

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove the .crash file - the .keep was there to create the folder in the first place. I need the empty folder for the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a suggestion how I can add an empty folder to git without the .keep file? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

This is fine. But why do you need an empty folder for the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct. I just need a folder without a "lobsterout" file to check that the validator is working. I will use one without lobsterout instead. Thanks.

@JaGeo
Copy link
Member Author

JaGeo commented Apr 27, 2020

Is there anything else I should do?

Best,
JG

@shyuep shyuep merged commit f9b4b8e into materialsproject:master Apr 27, 2020
@shyuep
Copy link
Member

shyuep commented Apr 27, 2020

Thanks.

@JaGeo
Copy link
Member Author

JaGeo commented Apr 27, 2020

Thank you very much. I forgot to ask: Would it be possible to release a new custodian version?

@shyuep
Copy link
Member

shyuep commented Apr 27, 2020

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shyuep shyuep shyuep left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JaGeo @shyuep