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

small-fixes #72

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

small-fixes #72

wants to merge 22 commits into from

Conversation

YAY-C
Copy link
Contributor

@YAY-C YAY-C commented Jun 30, 2024

@YAY-C YAY-C requested a review from briling July 2, 2024 12:46
@YAY-C YAY-C marked this pull request as ready for review July 2, 2024 12:46
@briling
Copy link
Contributor

briling commented Jul 2, 2024

could you please add a test for this?

qstack/spahm/rho/bond.py Outdated Show resolved Hide resolved
@YAY-C
Copy link
Contributor Author

YAY-C commented Dec 5, 2024

  • fix the input parsing
  • fix the spin evaluation (i.e. omods ARG)
  • ? move the dm computation to bond() ?

@YAY-C
Copy link
Contributor Author

YAY-C commented Dec 6, 2024

@briling checks failed because of numpy version I think this AttributeError
normally it should accept a callable since v. 1.23.0
Do you know if the version in the workflow is correct ?

  • EDIT : it's version clash between:
./pyproject.toml:    'numpy >= 1.22.3, < 1.27',
./environment.yml:  - numpy=1.21.2=py39h20f2e39_0

EDIT:
finally fixed in e1c9cca

Comment on lines -50 to -51
mols = utils.load_mols([xyz_in], [0], [None], 'minao', ecp='def2-svp')
dms = utils.mols_guess(mols, [xyz_in], 'LB', spin=[None])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was actually weird ! because even specifying spin=None the func would still returned merged alpha-beta representations (which is the X_true we loaded and evaluated) !

But it made me realized that it's a bit dangerous now too, because if you only use the bond() function with a single dm (for all electrons) and you don't change the parameters omods=[None] (default is [alpha, beta]) then you get an error ! (... I think ...)

Copy link
Contributor Author

@YAY-C YAY-C left a comment

Choose a reason for hiding this comment

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

The spin/omod thing was a mess ahaha
let me know what you think ! (I left a few comments)

Then I'll try to apply the same procedure to the atom-representation!

@@ -116,10 +113,13 @@ def get_repr(mols, xyzlist, guess, xc=defaults.xc, spin=None, readdm=None,
all_atoms = np.array([z for mol in mols for z in mol.elements if z in only_z], ndmin=2)
else:
all_atoms = np.array([mol.elements for mol in mols])
spin = np.array(spin) ## a bit dirty but couldn't find a better way to ensure Iterable type!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a pity that it has to be done twice when calling the main() script, but couldn't find any work around !

@YAY-C
Copy link
Contributor Author

YAY-C commented Dec 6, 2024

  • apply changes to atom.py

  • EDIT:
    or maybe merging ?

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.

SPAHM(b) generation
2 participants