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

Csse layout 550 error #458

Merged
merged 11 commits into from
Nov 26, 2024
Merged

Csse layout 550 error #458

merged 11 commits into from
Nov 26, 2024

Conversation

loriab
Copy link
Collaborator

@loriab loriab commented Nov 5, 2024

Description

  • see changelog

Changelog description

Status

  • Code base linted
  • Ready to go

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 99.29078% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.82%. Comparing base (0642655) to head (a7f030b).
Report is 7 commits behind head on next2024.

Additional details and impacted files

@loriab
Copy link
Collaborator Author

loriab commented Nov 12, 2024

Requesting review @davidbrownell, @krachwal, @rfievet, @ketanbj, @varun646, @jyoung3131

mdl = model_wrapper(data, qcelemental.models.v1.AtomicInput)
elif isinstance(data, qcelemental.models.v2.AtomicInput):
mdl = model_wrapper(data, qcelemental.models.v2.AtomicInput)
v1_model = getattr(qcelemental.models.v1, "AtomicInput")

Choose a reason for hiding this comment

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

Is there a reason to access AtomicInput this way rather than though an import? If so, it would be helpful to add a comment to explain why that is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a particularly good reason -- I switched in here in programs/model.py (where it's always AtomicInput) so the analog was clearer with procedures/model.py https://github.com/MolSSI/QCEngine/pull/458/files#diff-2e40dbc50862d2bd8716a45e9dafab44cb3be4499b972dc1d58b8144ef1ab046R63 where the flavor or Input isn't fixed.

@@ -7,7 +7,8 @@
from pathlib import Path
from typing import TYPE_CHECKING, Any, ClassVar, Dict

from qcelemental.models.v2 import AtomicResult, BasisSet
from qcelemental.models.v1 import AtomicResult as AtomicResult

Choose a reason for hiding this comment

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

It looks like as AtomicResult can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, that's the second time I've messed that line up. First time was something like ... import AtomicInput as AtomicResult, which was confusing. I've fixed this and added a comment for your previous comment in the next PR.

@@ -7,7 +7,8 @@
from pathlib import Path
from typing import TYPE_CHECKING, Any, ClassVar, Dict

from qcelemental.models.v2 import AtomicResult, BasisSet
from qcelemental.models.v1 import AtomicResult as AtomicResult
from qcelemental.models.v2 import BasisSet

Choose a reason for hiding this comment

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

Is BasisSet being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@loriab loriab mentioned this pull request Nov 13, 2024
3 tasks
@loriab loriab merged commit b84a9b0 into MolSSI:next2024 Nov 26, 2024
16 checks passed
@loriab loriab deleted the csse_layout_537a branch November 26, 2024 18:43
loriab added a commit that referenced this pull request Jan 18, 2025
* failedoperation.input_data as object when possible

* fix more tests, incl. d3/d4 return FailedOp and RandomError treated same as others

* fix the rest, incl. let dicts be cast to models based on schema_version field if present

* fix xtb

* fixes for py38 and dftd3

* fix for adcc

* test dftd3 on windows

* try windows and new d3

* fix

* test path

* regular testing lanes
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