Skip to content

Conversation

@miller-ian
Copy link
Contributor

@miller-ian miller-ian commented Jul 11, 2024

numpy + tox issues resolved with upgrade to numpy version 1.26.4

Tox py312 defaults to using numpy 2.0. Some numpy issues have been identified here and here. Dictating numpy version 1.26.4 (most recent version prior to 2.0) in tox.ini file resolves issues

@miller-ian miller-ian marked this pull request as draft July 11, 2024 01:54
@miller-ian miller-ian marked this pull request as ready for review July 11, 2024 02:01
@miller-ian miller-ian marked this pull request as draft July 11, 2024 02:14
@marcharper
Copy link
Member

Some of the types have changed, causing test failures like so:

  037     >>> import axelrod
  038     >>> axelrod.game.DefaultGame.RPST()
  Expected:
      (3, 1, 0, 5)
  Got:
      (np.int64(3), np.int64(1), np.int64(0), np.int64(5))

To be clear this appears to be due to numpy changes, not any of our type hints.

@miller-ian
Copy link
Contributor Author

Some of the types have changed, causing test failures like so:

  037     >>> import axelrod
  038     >>> axelrod.game.DefaultGame.RPST()
  Expected:
      (3, 1, 0, 5)
  Got:
      (np.int64(3), np.int64(1), np.int64(0), np.int64(5))

To be clear this appears to be due to numpy changes, not any of our type hints.

yep, this is an issue with tox + numpy. tox appears to default to using latest numpy version which, as you mention, gives us some problems. Reverting to numpy 1.26.4 seems to solve issue

pytest-sugar
isort
black
numpy==1.26.4
Copy link
Member

Choose a reason for hiding this comment

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

Should we pin this in pyproject.toml instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its in there as well!

Copy link
Member

Choose a reason for hiding this comment

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

In there it's not hard pinned. Just a lower bound. I wonder if it's worth hard pinning it.

Copy link
Member

Choose a reason for hiding this comment

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

We have avoided doing this in the past (I think mainly on my request?) but I wonder if it's worth starting to do (in our installation docs we do recommend using a venv etc...)...

We shouldn't delay merging this though, @miller-ian's contribution is a super helpful bug fix that lets other PRs get through :)

@drvinceknight
Copy link
Member

Thanks for this: it looks good to me.

@miller-ian miller-ian marked this pull request as ready for review July 11, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants