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

lint & format weights/*.py part 7 #655

Merged
merged 10 commits into from
Nov 16, 2023

Conversation

jGaboardi
Copy link
Member

@jGaboardi jGaboardi commented Nov 15, 2023

This PR:

  • formats and lints weights/*.py – part 7 of $n$

This is a bit more than I had thought it would be. Can split into 2 PRs if desired, @martinfleis

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #655 (f078de3) into main (c655e99) will not change coverage.
The diff coverage is 97.1%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #655   +/-   ##
=====================================
  Coverage   84.7%   84.7%           
=====================================
  Files        139     139           
  Lines      14859   14859           
=====================================
  Hits       12591   12591           
  Misses      2268    2268           
Files Coverage Δ
libpysal/weights/tests/test_adjlist.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test_contiguity.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test_distance.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test_nx.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test_raster.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test_spatial_lag.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test_spintW.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test_weights_IO.py 100.0% <100.0%> (ø)
libpysal/weights/tests/test__contW_lists.py 97.7% <97.2%> (-<0.1%) ⬇️
libpysal/weights/raster.py 62.4% <80.0%> (ø)

Comment on lines 566 to 568
attrs["nodatavals"] = tuple([fill_value])
attrs["nodatavals"] = fill_value
Copy link
Member

Choose a reason for hiding this comment

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

This changes the type, doesn't it? Can you test that?

Copy link
Member Author

Choose a reason for hiding this comment

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

So after looking into this a bit, there's a bit of strangeness going on with pytest-cov. The report claims those lines are covered, but after some tinkering locally it appears they are actually not. Going to investigate further.

Copy link
Member Author

Choose a reason for hiding this comment

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

So (unless I am missing something exceedingly obvious), indeed those lines were not being tested... and they fail with the change... BUT they also fail on main when actually tested... This adds another level of anxiety if we can't trust the coverage reports from pytest...

from libpysal.weights import raster
pytest.importorskip("xarray")
da2_missing_in = raster.testDataArray((1, 4, 4), missing_vals=True)
w2_missing = raster.da2W(da2_missing_in, "rook", n_jobs=-1)
da2_missing_out = raster.w2da(
    da2_missing_in.data.flatten(), w2_missing, da2_missing_in.attrs, None
)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[2], line 6
      4 da2_missing_in = raster.testDataArray((1, 4, 4), missing_vals=True)
      5 w2_missing = raster.da2W(da2_missing_in, "rook", n_jobs=-1)
----> 6 da2_missing_out = raster.w2da(
      7     da2_missing_in.data.flatten(), w2_missing, da2_missing_in.attrs, None
      8 )

File ~/libpysal/libpysal/weights/raster.py:343, in w2da(data, w, attrs, coords)
    341     raise TypeError("w must be an instance of weights.W")
    342 if hasattr(w, "index"):
--> 343     da = _index2da(data, w.index, attrs, coords)
    344 else:
    345     raise AttributeError(
    346         "This method requires `w` object to include `index` "
    347         "attribute that is built as a `pandas.MultiIndex` object."
    348     )

File ~/libpysal/libpysal/weights/raster.py:570, in _index2da(data, index, attrs, coords)
    568 else:
    569     data_complete = np.empty(shape, data.dtype)
--> 570 data_complete[indexer] = data
    571 coords = {}
    572 for dim, lev in zip(dims, idx.levels, strict=True):

ValueError: shape mismatch: value array of shape (16,) could not be broadcast to indexing result of shape (6,)

Copy link
Member Author

Choose a reason for hiding this comment

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

So I suppose I'll revert and create an issue for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that sounds the best

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we think all changes to raster should be reverted or just that one?

Copy link
Member

Choose a reason for hiding this comment

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

just this line and add noqa, the rest is fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@jGaboardi jGaboardi merged commit cf6b05e into pysal:main Nov 16, 2023
10 checks passed
@jGaboardi jGaboardi deleted the lint_format_weights_part_7 branch November 16, 2023 17:58
@jGaboardi jGaboardi changed the title lint & format weights/*.py part 7 lint & format `weights/*.py part 7 Nov 17, 2023
@jGaboardi jGaboardi changed the title lint & format `weights/*.py part 7 lint & format weights/*.py part 7 Nov 17, 2023
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