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

np version check fix #41

Merged
merged 3 commits into from
Oct 9, 2023
Merged

np version check fix #41

merged 3 commits into from
Oct 9, 2023

Conversation

JangidBhavnesh
Copy link
Contributor

@JangidBhavnesh JangidBhavnesh commented Oct 8, 2023

The load_library(libname) function at https://github.com/MatthewRHermes/mrh/blob/master/lib/helper.py has a weird way of checking the np version. Like, if condition is becoming true if np version is 1.21.6.

In this PR, I am adding a fix to make it consistent so that this condition becomes true for the np version less than or equal to the 1.6.

Thank You

@MatthewRHermes
Copy link
Owner

I don't think that quite works the way you want it to:

>>> a = tuple (map (int, '1.6'.split ('.')))
>>> b = tuple (map (int, '1.6.1'.split ('.')))
>>> b<=a
False
>>> '1.6' in '1.6.1'
True

Should it just check that the first two elements of the tuple are exactly '1', '6'?

@MatthewRHermes
Copy link
Owner

MatthewRHermes commented Oct 9, 2023

BTW this is so old that the corresponding code no longer exists in PySCF itself, probably because the maintainers gave up on supporting numpy 1.6

@JangidBhavnesh
Copy link
Contributor Author

**Current code fails for numpy version 1.21.6

'1.6' in '1.21.6'
True

I tried to overcome that by using above function. But clearly that doesn't seem the correct way.**

With new commit, I think it will be overcome for any np version less than 1.6.2

version_tuple('1.6.0') <= version_tuple('1.6.2')
True
version_tuple('1.5.0') <= version_tuple('1.6.2')
True
version_tuple('1.11.0') <= version_tuple('1.6.2')
False
version_tuple('1.3.0') <= version_tuple('1.6.2')
True
version_tuple('1.1.0') <= version_tuple('1.6.2')
True
version_tuple('1.6') <= version_tuple('1.6.2')
True

I know this isn't the best way but it is solving the problem. If you have any other suggestion then please let me know.

Thank you

@MatthewRHermes
Copy link
Owner

I think it was specifically 1.6 that had the bug, in fact at one point a PySCF warning recommended you downgrade if you had 1.6. Should it just check that the first two elements of the tuple are exactly '1', '6'?

@JangidBhavnesh
Copy link
Contributor Author

So, Instead of less than 1.6 version, I will make it for 1.6.x only.

version_tuple('1.1.1') == version_tuple('1.6.2')
False
version_tuple('1.1.2') == version_tuple('1.6.2')
False
version_tuple('1.1') == version_tuple('1.6.2')
False
version_tuple('1.6') == version_tuple('1.6.2')
True
version_tuple('1.7') == version_tuple('1.6.2')
False

How does this look?

@MatthewRHermes
Copy link
Owner

I don't think it's the clearest thing in the world when you define something called "version_tuple" that throws away part of the version tuple. Can you just index on the equality, rather than the function definition? It's less efficient but it's clearer to read.

@MatthewRHermes
Copy link
Owner

P.S. ignore my deleted comment, I wasn't looking closely enough before I sent that.

@MatthewRHermes MatthewRHermes merged commit ae5125d into MatthewRHermes:dev Oct 9, 2023
3 checks passed
@MatthewRHermes
Copy link
Owner

Whatever I'll just do it myself

MatthewRHermes added a commit that referenced this pull request Oct 18, 2023
cjknight pushed a commit to cjknight/mrh that referenced this pull request Nov 14, 2023
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