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

Added dataclasses decorator to molecule class #114

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Added dataclasses decorator to molecule class #114

wants to merge 34 commits into from

Conversation

TL231
Copy link
Contributor

@TL231 TL231 commented Oct 4, 2024

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.

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (43084fb) to head (6cf2dd0).
Report is 25 commits behind head on main.

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     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@damarkian damarkian requested a review from chmwzc November 5, 2024 04:40
@chmwzc
Copy link
Contributor

chmwzc commented Dec 3, 2024

Any other changes that you want to make? If not, looks good to me, you can un-draft it and I'll merge it 👍

@TL231
Copy link
Contributor Author

TL231 commented Dec 3, 2024

There's another way to enforce class attribute type using the attr package link.
AFAIK, it should not affect anything. I'll try to implement it by this week, but if it fails, I'll undraft this to be merged.

@TL231
Copy link
Contributor Author

TL231 commented Dec 6, 2024

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 validator=attr.validators.instance_of(type) in attr.ib.

@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.
_process_xyz_file has been subsume under attrs_post_init which automatically runs after init.

@TL231 TL231 changed the title change test_ucc line 246 tuple to int Added attrs support to molecule class Jan 6, 2025
@TL231
Copy link
Contributor Author

TL231 commented Jan 8, 2025

  • Use dataclasses instead of attrs (refer to link).

@TL231 TL231 changed the title Added attrs support to molecule class Added dataclasses decorator to molecule class Feb 6, 2025
@TL231
Copy link
Contributor Author

TL231 commented Feb 6, 2025

@chmwzc
Hi, nearly done, but I just need your help to look through the molecule class and change the value attribute types to correct ones.
active: None = None should be like active: list = None instead?

@chmwzc
Copy link
Contributor

chmwzc commented Feb 6, 2025

@chmwzc Hi, nearly done, but I just need your help to look through the molecule class and change the value attribute types to correct ones. active: None = None should be like active: list = None instead?

Sure, but before that, it seems like you're allowing for all the attributes to be defined in __init__? (correct me if I'm wrong)
We only need a few of them (Currently: def __init__(self,(geometry=None, charge=0, multiplicity=1, basis=None, xyz_file=None, active=None)), the rest will be populated when necessary.

@TL231
Copy link
Contributor Author

TL231 commented Feb 6, 2025

Sure, but before that, it seems like you're allowing for all the attributes to be defined in __init__?

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 example,
Molecule([("H", (0.0, 0.0, 0.0)), ("H", (0.0, 0.0, 0.7))])
Will take "H", (0.0, 0.0, 0.0)), ("H", (0.0, 0.0, 0.7) as the input for geometry. However, if I move the geometry attributew to the end of attribute list in the dataclass, "H", (0.0, 0.0, 0.0)), ("H", (0.0, 0.0, 0.7) will now be assigned to charge causing an error.
Defining all the attribute lets us keep the order which preserves existing workflows.

@TL231 TL231 marked this pull request as ready for review March 6, 2025 09:32
@TL231
Copy link
Contributor Author

TL231 commented Mar 6, 2025

@chmwzc
Slipped my mind, so long overdue, but can check if the additional parameters still pop up?

@chmwzc
Copy link
Contributor

chmwzc commented Mar 7, 2025

Thanks for the changes, I'll edit the default attribute types after my comments are addressed!

Comment on lines +43 to +46
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
Copy link
Contributor

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?

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