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

Fix factorial2 #138

Merged
merged 2 commits into from
Nov 18, 2023
Merged

Fix factorial2 #138

merged 2 commits into from
Nov 18, 2023

Conversation

FarnazH
Copy link
Member

@FarnazH FarnazH commented Nov 17, 2023

Steps

  • Write a good description of what the PR does.
  • Add tests for each unit of code added (e.g. function, class)
  • Update documentation
  • Squash commits that can be grouped together
  • Rebase onto master

Description

This PR addresses issue #129 by adding a factorial2 function that wraps scipy.special.factorial2 to return 1.0 when the input is not positive. This is temporary additional, while we wait for Scipy's update. To learn more, see scipy/scipy#18409. This function is added in gbasis.utils (I tried adding it to gbasis.wrappers first but that would cause circular imports).

Just patching scipy version (as was done in f74123e) was not enough, because unless you install the gbasis package, the correct version of scipy is not used causing headache. Also, one might need to use scipy 1.11.x in their environment for other purposes. In the meanwhile, correcting the factorial2 output seems to me a more practical solution than patching the scipy version.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #129

Copy link
Member

@PaulWAyers PaulWAyers left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @FarnazH

Are there any other pieces of gbasis that really belong in utils.py? I can't think of any right now, but probably eventually some other things (e.g., functions related to the Boys function) belong there.

@FarnazH
Copy link
Member Author

FarnazH commented Nov 17, 2023

@PaulWAyers, I cannot think of any other function right now, but it is good to keep in mind.

@leila-pujal leila-pujal merged commit 5fcd190 into master Nov 18, 2023
@FarnazH FarnazH deleted the fix_factorial2 branch November 18, 2023 02:50
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.

Codes using factorial2 function
4 participants