Skip to content

Conversation

@YAY-C
Copy link
Contributor

@YAY-C YAY-C commented Jan 21, 2026

due to rounding error in computed cosine values (see get_cos())

  • clip value returned by get_cos() to range domain [-1, 1]
  • (misc) add verbosity level to e-SPAHM compute script

@YAY-C YAY-C requested review from briling and liam-o-marsh January 21, 2026 21:14
@YAY-C
Copy link
Contributor Author

YAY-C commented Jan 22, 2026

@briling sorry I just also included some verbosity level to suppress some warnings because I'm using these functions in a notebook and its a bit annoying (also it conflicts with the progress bar)
shouldn't change the normal behaviour!

@YAY-C YAY-C requested a review from briling January 22, 2026 11:27
@briling
Copy link
Contributor

briling commented Jan 22, 2026

the second commit belongs to a different PR i think...

but importantly, it changes the behavior, because the check_nelec() function also raises an error if there is too many electrons to fit into existing basis. we added it after fixing the minao-without-ecp bug for Iodine, and it should catch similar bugs for other bases. so the options are:

  1. rename the argument to ignore_too_many_electrons/allow_too_many_electrons or something (so it looks dangerous) but i think it's still dangerous
  2. pass this verbose argument to check_nelec() so the error is raised but the warning is suppressed
  3. option 2) but rename it to suppress_full_shell_warning or something (i think verbose is too vague)

Copy link
Contributor

@briling briling left a comment

Choose a reason for hiding this comment

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

(see the comment)

@briling
Copy link
Contributor

briling commented Jan 22, 2026

why do you have full-shell molecules anyway?

@liam-o-marsh
Copy link
Contributor

I'm partial to a modified option 3 ("suppress_warnings", in case other warnings get added in the future)

@YAY-C YAY-C mentioned this pull request Jan 26, 2026
3 tasks
@YAY-C
Copy link
Contributor Author

YAY-C commented Jan 26, 2026

@briling @liam-o-marsh
I moves the warning discussion to a new PR and will merge the arccos() fix that you approved!

@YAY-C YAY-C merged commit f88d45c into master Jan 26, 2026
16 checks passed
@briling briling deleted the fix_arccos branch January 27, 2026 16:53
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.

4 participants