-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added dataclasses decorator to molecule class #114
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 751 752 +1
=========================================
+ Hits 751 752 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Any other changes that you want to make? If not, looks good to me, you can un-draft it and I'll merge it 👍 |
There's another way to enforce class attribute type using the attr package link. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Seems to be working, the remaining errors are from the #119. To note, although it is not listed, the class still accepts variables in the order they are listed in the class. Most of the variables are init as None without typing, and adding a type check is as simple as adding @chmwzc please help do a quick scan of the class and add appropriate validators if needed, do note that the validators will check the default and return an error if there's type mismatch. Leave out the validators if there's multiple acceptable typing. |
|
@chmwzc |
Sure, but before that, it seems like you're allowing for all the attributes to be defined in |
That's an artifact of how dataclass works. It doesn't allow me to write attributes with defaults before variables without defaults. If I try to move those few initially defined attributes to the bottom, that messes with existing workflow. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@chmwzc |
Thanks for the changes, I'll edit the default attribute types after my comments are addressed! |
for more information, see https://pre-commit.ci
active: None = None #: Iterable of molecular orbitals included in the active space | ||
frozen: None = field( | ||
default=None, init=False | ||
) #: Iterable representing the occupied molecular orbitals removed from the simulation |
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.
These attributes (active
and frozen
) should come after the nelec, norb, ... class attributes in the docs. Should be OK to just move them there?
The class attributes of the Molecule class does not enforce types, run_pyscf also overrides the attributes, so the tuple is accepted without issues.
Shouldn't pass the ucc_ansatz due to typing issues though.
Regarding type enforcement, see link.
tl;dr is that enforcing type on class attributes during init is not recommended, if type enforcement is desired, we can follow the example in one of the answers.