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

QCSchema coordination, testing, and export #237

Merged
merged 10 commits into from
Jan 30, 2021
Merged

Conversation

loriab
Copy link
Collaborator

@loriab loriab commented Sep 4, 2020

Description

This reconciles some details of models that had gotten out of sync with qcsk. It makes the schema generatable from the models. It dumps some examples of schema from the existing test cases. It patches up some mypy errors. Finally, it starts setting up some coordination with qcsk so that PRs to qcel (on upstream) that propose to change the schema get notified over in qcsk, addressing MolSSI/QCSchema#68 . This coordinates with MolSSI/QCSchema#77

Changelog description

  • export models to JSON Schema with make qcschema
  • generate example json from tests and test against exported schema with pytest --validate qcelemental/
  • mypy fixes incl. dynamic provenance. bump pydantic to v1.5
  • testing configuration and GHA coordination with QCSchema repo
  • material changes to models
  • description string and formatting changes to models
  • a0 as Bohr abbreviation. test basis sets as str or float
  • some GHA details aren't polished yet.
    • to have permissions to push a diff branch to qcsk, model-changing PRs here need to be on upstream, not forks
    • need to be able to detect no-change PRs so they're still allowed from forks
    • I turned off CI checks to non-master to avoid duplicate checks, and now the Required ones aren't satisfied

Status

  • Code base linted
  • Ready to go

@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #237 (b970ecb) into master (217e650) will decrease coverage by 0.24%.
The diff coverage is 89.50%.

@loriab
Copy link
Collaborator Author

loriab commented Sep 8, 2020

@dgasmith, a few changes to qcel that you should probably glance over to see if they ring any alarm bells. particularly the first bullet.

Copy link
Collaborator

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

  1. I don't think exclude_defaults was an option at the time.
  2. Thats fine, but I did like in the past where it forced you to add these.
  3. Sounds good, but will require a lot of checks inside QCEngine.
  4. Very much needed.
  5. The mp2 issues isn't linking at the moment, I cannot see what you are referring to.
  6. The aliases are fine, though I'm not sure E_h will work as expected. I don't remember if there are odd rules around this.
  7. The .json link doesn't appear to be working.

setup.py Outdated
@@ -31,7 +31,7 @@
package_data={'': [os.path.join('qcelemental', 'data', '*.json')]},
setup_requires=[] + pytest_runner,
python_requires='>=3.6',
install_requires=['numpy >= 1.12.0', 'pint >= 0.10.0', 'pydantic >= 1.0.0'],
install_requires=["numpy >= 1.12.0", "pint >= 0.10.0", "pydantic >= 1.5.0"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pydantic 1.6.0 has a bad regression with nested models, recommend skipping. Fixed in 1.6.1.

AtomicResultProperties,
BasisSet,
Molecule,
Provenance,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optimization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Optimization was never added over at qcsk, so I was aiming for a 1:1 changeset. Agree that it'd be good to get OptimizationInput/Result formalized over there soon.

@loriab
Copy link
Collaborator Author

loriab commented Sep 9, 2020

  1. I don't think exclude_defaults was an option at the time.

✔️

  1. Thats fine, but I did like in the past where it forced you to add these.

I don't think it was, though. It's been None, not .... https://github.com/MolSSI/QCElemental/blob/master/qcelemental/models/common_models.py#L23-L25 The latter I would have left alone as being consistent with qcsk's all three of creator/version/routine being required. Glad to switch to ... instead of "".

  1. Sounds good, but will require a lot of checks inside QCEngine.

yes, indeed

  1. Very much needed.

✔️

  1. The mp2 issues isn't linking at the moment, I cannot see what you are referring to.

Maybe this works better https://github.com/MolSSI/QCElemental/blob/master/qcelemental/models/results.py#L77-L79, mp2_total_correlation_energy

  1. The aliases are fine, though I'm not sure E_h will work as expected. I don't remember if there are odd rules around this.

Ok. I haven't explored Datum with these aliases either, so was trying for nominal consistency

  1. The .json link doesn't appear to be working.

fp.write(instance.json(exclude_unset=True, exclude_none=True)) aka https://github.com/MolSSI/QCElemental/pull/237/files#diff-37eecce1557470bdbfa6da71286b3090R66

Copy link
Collaborator

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

Thanks for the links, it all looks great!

@loriab
Copy link
Collaborator Author

loriab commented Sep 9, 2020

It turns out a0/a_0/Eh/E_h are already in pint c. v0.10 hgrecco/pint#811

python -c "exec(\"import pathlib, qcelemental\nfor md in qcelemental.models.qcschema_models():\n\tmfile = (pathlib.Path('qcschema') / md.__name__).with_suffix('.schema')\n\twith open(mfile, 'w') as fp:\n\t\tfp.write(md.schema_json(indent=None))\")"
python -c "exec(\"import json, pathlib, pydantic, qcelemental\nwith open((pathlib.Path('qcschema') / 'QCSchema').with_suffix('.schema'), 'w') as fp:\n\tjson.dump(pydantic.schema.schema(qcelemental.models.qcschema_models(), title='QCSchema'), fp, indent=4)\")"
@loriab loriab merged commit 7fb730e into MolSSI:master Jan 30, 2021
@loriab loriab deleted the schemalint13 branch January 30, 2021 03:45
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.

2 participants