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 the firework for calculating the electronic energy of a proton in different solvent environments[WIP] #758

Merged
merged 2 commits into from
Jul 14, 2022

Conversation

rdguha1995
Copy link
Contributor

@rdguha1995 rdguha1995 commented Jun 26, 2022

Summary

For this custom Firework the electronic energy of a proton in a specific solvent environment is approximated. Since a proton has 0 electrons, running a QChem job would yield an error. The energy can be approximated by:

  • Calculating the energy of a hydrogen atom with 0 charge
  • Calculating the energy of a system with a hydrogen atom (0 charge) and a proton separated at infinite distance
  • Subtracting the first energy from the second

This Firework combines these two calculations and adds a task doc to the DB with the separate calculation details and the effective energy after subtraction.

TODO (if any)

  • Should be reviewed by @samblau before merge
  • Needs a CI test run.

@rdguha1995 rdguha1995 changed the title added the proton electronic energy firework Add the firework for calculating the electronic energy of a proton in different solvent environments[WIP] Jun 26, 2022
@samblau
Copy link
Contributor

samblau commented Jun 28, 2022

@Zhuoying, please trigger the tests here

@Zhuoying
Copy link
Contributor

Zhuoying commented Jul 1, 2022

Thanks @samblau for the reminder. -Just trigger tests and it looks like passing the tests.
Since it is related to QChem jobs, could you please review it when you are available?
Please free feel to ping me if you need anything from my side. Thanks.

Copy link
Contributor

@samblau samblau left a comment

Choose a reason for hiding this comment

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

Very minor changes suggested. Once they are made, this should be ready to merge!

"output_file_H2",
"additional_fields",
"db_file",
"multirun",
Copy link
Contributor

Choose a reason for hiding this comment

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

multirun should not be an optional parameter here since we are prescribing exactly what is being parsed

additional_fields (dict): dict of additional fields to add
db_file (str): path to file containing the database credentials.
Supports env_chk. Default: write data to JSON file.
multirun (bool): Whether the job to parse includes multiple
Copy link
Contributor

Choose a reason for hiding this comment

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

remove multirun documentation

output_file_H0 = self.get("output_file_H0", "H0.qout")
input_file_H2 = self.get("input_file_H2", "H2_plus.qin")
output_file_H2 = self.get("output_file_H2", "H2_plus.qout")
multirun = self.get("multirun", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove multirun

path=calc_dir,
input_file=input_file_H0,
output_file=output_file_H0,
multirun=multirun,
Copy link
Contributor

Choose a reason for hiding this comment

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

multirun=false

path=calc_dir,
input_file=input_file_H2,
output_file=output_file_H2,
multirun=multirun,
Copy link
Contributor

Choose a reason for hiding this comment

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

multirun=false

@@ -132,6 +136,72 @@ def test_SinglePointFW_not_defaults(self):
self.assertEqual(firework.parents, [])
self.assertEqual(firework.name, "special single point")

def test_ProtonEnergyFW_not_defaults(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this test is using all default values, so it should be named "test_ProtonEnergyFW_defaults"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @samblau for the comments! I have implemented them and I think this should be ready to merge. Thanks @Zhuoying for all the help.

@rdguha1995
Copy link
Contributor Author

Hey @Zhuoying! It would be great if you can take some time to merge this. I have implemented all the changes suggested by @samblau

@rdguha1995
Copy link
Contributor Author

Hey @Zhuoying . Just circling back on this. Can you take some time out for merging this?

Thanks a lot!

@Zhuoying
Copy link
Contributor

@rdguha1995 Thanks for the reminder. I am re-trigger the tests and will merge it after all tests pass.

@rdguha1995
Copy link
Contributor Author

Hey @Zhuoying , can you help me with the current errors? They are not in codes I have played with. What would be the best way to solve this?

@Zhuoying
Copy link
Contributor

@rdguha1995 Yes, the error seems to be induced by the recent change in pymatgen.io.feff (https://github.com/materialsproject/pymatgen/blob/c1e47a78bcf5d4f23b11b83570adb861f47cb183/pymatgen/io/feff/sets.py#L357), not your part.
I will merge your PR for now. And fix the error in another PR. Thanks!

@Zhuoying Zhuoying merged commit 3b5da4e into hackingmaterials:main Jul 14, 2022
@janosh
Copy link
Member

janosh commented Jul 27, 2022

CI fix submitted in #760.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants