-
Notifications
You must be signed in to change notification settings - Fork 46
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
Rename json to json_qcschema #360
Conversation
Reviewer's Guide by SourceryThis pull request renames the JSON schema to File-Level Changes
Tips
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tovrstra - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
I checked the deepsource issues. Some of them should be fixed eventually, but they are not related to this PR. I'm not fixing them now to keep this PR manageable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me.
I think that it isn't worth working too hard on this, as I think QCSchema may be basically dead (no activity in >3 years).
Thanks for checking. I agree, I'm just keeping it alive for now and avoid that it blocks us from doing other JSON-related things in the future. |
There was some recent activity in issues, but yes. The schema proper in this repository is also outdated; the latest version is generated on-the-fly from Pydantic models in QCElemental. |
@berquist Thank you for the info. So, if I understand correctly, the following files are no longer the right ones? https://github.com/MolSSI/QCSchema/tree/master/qcschema/data/v2 (If yes, is there another way to get up-to-date schema files? That would be useful.) |
That's correct. This should give you what you want if you comment out all the """dump_schemas.py: Save the JSON representation of each schema stored in QCSchema and QCElemental."""
from pathlib import Path
import json
# This requires https://github.com/MolSSI/QCSchema/pull/81
from qcschema.dev import dev_schema
import qcelemental as qcel
base_dir = Path(__file__).parent.resolve()
schema_dir = base_dir / "schemas"
schema_dir.mkdir(mode=0o755, parents=True, exist_ok=True)
qcel_models = {
"qcel_atomic_input": qcel.models.results.AtomicInput,
"qcel_atomic_result": qcel.models.results.AtomicResult,
"qcel_atomic_result_properties": qcel.models.results.AtomicResultProperties,
"qcel_basis_set": qcel.models.basis.BasisSet,
"qcel_molecule": qcel.models.molecule.Molecule,
"qcel_provenance": qcel.models.common_models.Provenance,
}
for name, model in qcel_models.items():
(schema_dir / f"{name}.json").write_text(json.dumps(model.schema(), indent=2), encoding="utf-8")
qcsk_models = {
"qcsk_input": dev_schema.input_dev_schema,
"qcsk_output": dev_schema.output_dev_schema,
"qcsk_basis_set": dev_schema.basis_dev_schema,
"qcsk_molecule": dev_schema.molecule_dev_schema,
}
for name, model in qcsk_models.items():
(schema_dir / f"{name}.json").write_text(json.dumps(model, indent=2), encoding="utf-8") |
@berquist Thank you for providing the script. I'll open a new issue, so we can keep track of it. I'm currently prioritizing API stability for IOData, so I will not be able to get on it in the short term. |
This is one of the cleanups listed in #204.
There are several JSON schemas that could be relevant for IOData, and so we should rename our current one to something more specific, so we don't have to revise the API when adding a second JSON schema. The ones I'm aware of include:
I've also removed the
*.json
pattern, so one has to specify explicitly which JSON schema to use.It seemed better to use
json_qcshema.py
thanqcschema_json.py
. When another JSON schema module is added, it will be alphabetically close, making it easier to spot in the list of modules.Edit: I've added some tests suggested by sourcery AI, but then also had to fix some error handingling in the api module to make the tests pass.
Summary by Sourcery
This pull request renames the existing JSON schema module to 'json_qcschema' to facilitate the addition of other JSON schemas in the future. It also updates the test cases to use the new format explicitly.