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

[MAINT] lint & format repo; update CI; handle DeprWarns, etc. #128

Merged
merged 17 commits into from
Oct 26, 2022

Conversation

jGaboardi
Copy link
Member

@jGaboardi jGaboardi commented Oct 26, 2022

This PR:

  • lints and formats the mapclassify repo
    • several variables renamed due to flake8 complaining
    • several variables/code chunks removed/commented out due to being flagged by flake8 as not used
  • add a pre-commit hook for formatting & listing
  • drop support/testing for Python 3.7
  • add linting to CI
  • cleans up testing for warnings
  • rerun notebooks
  • cleans up and add doctests to CI run, and adapts for windows
  • add random seed keyword args to classifiers.MaxP

Still needs attention: (probably handle in a separate PR)

mapclassify/tests/test_mapclassify.py::TestUserDefined::test_UserDefined_invariant
  /Users/user/mapclassify/mapclassify/classifiers.py:908: RuntimeWarning: invalid value encountered in double_scalars
    gadf = 1 - self.adcm / Adam

xref: geopandas/geopandas#2553

@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #128 (ff5c0a6) into master (1887b32) will increase coverage by 4.2%.
The diff coverage is 87.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #128     +/-   ##
========================================
+ Coverage    78.0%   82.2%   +4.2%     
========================================
  Files          12       7      -5     
  Lines        1936    1118    -818     
========================================
- Hits         1511     919    -592     
+ Misses        425     199    -226     
Impacted Files Coverage Δ
mapclassify/classifiers.py 80.9% <81.0%> (+1.6%) ⬆️
mapclassify/_classify_API.py 93.1% <93.8%> (ø)
mapclassify/datasets/calemp/data.py 100.0% <100.0%> (ø)
mapclassify/greedy.py 91.4% <100.0%> (ø)
mapclassify/pooling.py 78.9% <100.0%> (-0.5%) ⬇️
mapclassify/__init__.py
mapclassify/_version.py
mapclassify/tests/test_classify.py
mapclassify/tests/test_greedy.py
mapclassify/tests/test_mapclassify.py
... and 1 more

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.

looks mostly okay to me, thanks!

.coveragerc Outdated Show resolved Hide resolved
@jGaboardi
Copy link
Member Author

looks mostly okay to me, thanks!

Need to make sure these tests are all passing. Really weird that tests only failed for macOS Py3.10 (which is what I'm running locally, of course passing). Hopefully the additional random seed keywords solve the failures.

@jGaboardi
Copy link
Member Author

mostly okay

"mostly okay" is not a ringing endorsement. LOL. Anything specific to change/not change?

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.

"mostly okay" referred to that one comment I left in the code :).

@jGaboardi jGaboardi merged commit 12ac854 into pysal:master Oct 26, 2022
@jGaboardi jGaboardi deleted the admin_work branch October 26, 2022 17:53
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.

2 participants