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

Add type signatures to jenksy #119

Merged
merged 6 commits into from
Feb 2, 2022
Merged

Add type signatures to jenksy #119

merged 6 commits into from
Feb 2, 2022

Conversation

cheginit
Copy link

Per the discussion in #118, this PR does the following:

  • Add numba type signatures to _fisher_jenks_means
  • Remove sort argument from _fisher_jenks_means since it's not being passed from the class function and is always True.
  • Add a new extra_req to setup.py for installing numba as speedups.
  • Add __pycach__ to .gitignore since numba compiles the function and keep in the cache folder.
  • Warn the user if numba is not installed the (slow) pure Python code is used.

@cheginit
Copy link
Author

Two of the greedy tests fail and since they are not related to this PR I didn't attempt to address them.

@martinfleis martinfleis reopened this Dec 10, 2021
@martinfleis
Copy link
Member

(sorry for closing and reopening, I wanted to make sure CI kicks in)

@cheginit
Copy link
Author

Sure. No problem! The tests in the CI are passing. However, I used Python 3.10 and latest versions of the dependencies to run pytest on my machine and two of the greedy tests failed. Looking at the source code, the problem seems to be with the latest version of networkx. The assertions fail.

@cheginit
Copy link
Author

I used tox-conda with the following config and all tests pass. I am not sure why three of the CI's were stuck for 3 hours.

[tox]
envlist =
    {py36,py37,py38}-{speedup,slow}

[testenv]
conda_deps=
    pytest
    pytest-cov
    codecov
    geopandas
    libpysal
    networkx
    palettable
    speedup: numba
commands=
    pytest {posargs}

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

The PR looks okay to me! Thanks!

However, I used Python 3.10 and latest versions of the dependencies to run pytest on my machine and two of the greedy tests failed.

You can ignore that. I'll make a PR slimming down our CI matrix and including python 3.10 and look into this.

I am not sure why three of the CI's were stuck for 3 hours.

don't know either

@martinfleis
Copy link
Member

two of the greedy tests failed.

See networkx/networkx#3993

@cheginit
Copy link
Author

Oh. So the issue has been there for a while.

Sounds good! Let me know if this PR needs any other changes.

@cheginit
Copy link
Author

cheginit commented Feb 2, 2022

It's been a while since I opened this PR, do I need to change anything in this PR?

@sjsrey sjsrey merged commit 3288c94 into pysal:master Feb 2, 2022
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.

3 participants