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.

Sign up for GitHub

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

Jump to bottom

Fix NumPy array indexing in 32-bit system in analysis.chemenv.coordination_environments #4194

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

Fix NumPy array indexing in 32-bit system in analysis.chemenv.coordination_environments #4194

wants to merge 2 commits into from

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 24, 2024 •
edited
Loading

Summary

I may use intp instead of uintp as I'm not 100% sure if the indexes could be negative or not in the code design.

Run unit tests locally as GitHub doesn't provide 32-bit runner

Test platform:

  • Linux debian32bit 6.1.0-27-686-pae #1 SMP PREEMPT_DYNAMIC Debian 6.1.115-1 (2024-11-01) i686 GNU/Linux
  • Python 3.11.2

Numpy 1.26.4

I don't think those three test failures are related to this PR.

.....................................ss...................................................................................................... [  4%]
.................................s.............................sss.......................................F........sssss.....s..s............. [  9%]
.....s..............................................ss.......................s.....ssssssss.................................................. [ 14%]
.......s...............................................................................................................ss.................... [ 19%]
.................................F....................................................sss....s.s............................................. [ 24%]
....................sssss..ss..sssssssssssssssssssssssssssssssssssssss....................................................................... [ 29%]
...................................................F......................................................................................... [ 33%]
...................................................s.......s..................................sss....s...............sssssss..........s...... [ 38%]
............................ss.....sss....s..........................s....................................................................... [ 43%]
.s.................................ssssssssssssssss...................................................................................sssssss [ 48%]
ssssssssssss................................................................................................................................. [ 53%]
..................................................s...................ss..................................................................... [ 58%]
............................................................................................................................................. [ 62%]
............................................................................................................................................. [ 67%]
.............ssss................................................s..................s........................................................ [ 72%]
...................................................................................ss............................sssss..s.................... [ 77%]
...........................................................................................................s................................. [ 82%]
............................................................................................................................................. [ 87%]
...............................................s.......................................................................sssssssssss........... [ 91%]
..............................................................................s................s..sssss..ssssssss...ssssss....s...........s.. [ 96%]
.................ssss..........................................................................                                               [100%]
===================================================================== FAILURES ======================================================================
_________________________________________________________ TestEOS.test_numerical_eos_values _________________________________________________________

self = <analysis.test_eos.TestEOS testMethod=test_numerical_eos_values>

    def test_numerical_eos_values(self):
        assert_allclose(self.num_eos_fit.e0, -10.84749, atol=1e-3)
        assert_allclose(self.num_eos_fit.v0, 40.857201, atol=1e-1)
        assert_allclose(self.num_eos_fit.b0, 0.55, atol=1e-2)
>       assert_allclose(self.num_eos_fit.b0_GPa, 89.0370727, atol=1e-1)

/home/yang/pymatgen/tests/analysis/test_eos.py:429: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = (<function assert_allclose.<locals>.compare at 0x9a968d48>, array(88.86413), array(89.0370727))
kwds = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=1e-07, atol=0.1', 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
E           AssertionError: 
E           Not equal to tolerance rtol=1e-07, atol=0.1
E           
E           Mismatched elements: 1 / 1 (100%)
E           Max absolute difference: 0.1729427
E           Max relative difference: 0.00194237
E            x: array(88.86413)
E            y: array(89.037073)

/usr/lib/python3.11/contextlib.py:81: AssertionError
__________________________________________________________ TestBalancedReaction.test_hash ___________________________________________________________

self = <analysis.test_reaction_calculator.TestBalancedReaction testMethod=test_hash>

    def test_hash(self):
>       assert hash(self.rxn) == 4774511606373046513
E       assert 221359835 == 4774511606373046513
E        +  where 221359835 = hash(24 Li + Na2S + 3 K2SO4 -> 2 KNaS + 2 K2S + 12 Li2O)
E        +    where 24 Li + Na2S + 3 K2SO4 -> 2 KNaS + 2 K2S + 12 Li2O = <analysis.test_reaction_calculator.TestBalancedReaction testMethod=test_hash>.rxn

tests/analysis/test_reaction_calculator.py:334: AssertionError
_______________________________________________________________ TestLattice.test_init _______________________________________________________________

self = <core.test_lattice.TestLattice testMethod=test_init>

    def test_init(self):
        len_a = 9.026
        lattice = Lattice.cubic(len_a)
        assert lattice is not None, "Initialization from new_cubic failed"
        assert_array_equal(lattice.pbc, (True, True, True))
>       assert hash(lattice) == -6887896986157825384
E       assert 50099439 == -6887896986157825384
E        +  where 50099439 = hash(Lattice\n    abc : 9.026 9.026 9.026\n angles : 90.0 90.0 90.0\n volume : 735.336269576\n      A : 9.026 0.0 0.0\n      B : 0.0 9.026 0.0\n      C : 0.0 0.0 9.026\n    pbc : True True True)

/home/yang/pymatgen/tests/core/test_lattice.py:43: AssertionError

3 failed, 2719 passed, 205 skipped, 54 warnings in 1223.52s (0:20:23)

NumPy 2

Cannot install:

ERROR: Cannot install monty==2024.10.21, pymatgen and pymatgen==2024.11.13 because these package versions have conflicting dependencies.

The conflict is caused by:
    pymatgen 2024.11.13 depends on numpy<3 and >=2.0.0
    matplotlib 3.9.0 depends on numpy>=1.23
    monty 2024.10.21 depends on numpy<2.0.0

@DanielYang59 DanielYang59 marked this pull request as ready for review November 24, 2024 12:19
@DanielYang59
Copy link
Contributor Author

@drew-parsons I believe this resolved the index dtype issue in 32-bit systems (tested on Debian 12 32-bit with NumPy 1.26.4 and Python 3.11), let me know if you notice any issues :)

@drew-parsons
Copy link
Contributor

Thanks. This patch seems to be working well, applied to debian build 2024.10.29+dfsg1-4
https://buildd.debian.org/status/package.php?p=pymatgen

@DanielYang59
Copy link
Contributor Author

No problem, great to know, thanks for confirming this!

@DanielYang59 DanielYang59 changed the title Fix NumPy array indexing in 32-bit system Fix NumPy array indexing in 32-bit system in analysis.chemenv.coordination_environments Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shyuep shyuep Awaiting requested review from shyuep shyuep is a code owner

@mkhorton mkhorton Awaiting requested review from mkhorton mkhorton is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

coordination_geometry int64 indices breaks on 32-bit systems
2 participants
@DanielYang59 @drew-parsons