Skip to content

shading: use morphology.footprint_rectangle((1, 3)) for 1x3 structuring element#233

Merged
cwhanse merged 6 commits into
pvlib:mainfrom
Omesh37:fix/skimage-rectangle-deprecated
Jul 8, 2026
Merged

shading: use morphology.footprint_rectangle((1, 3)) for 1x3 structuring element#233
cwhanse merged 6 commits into
pvlib:mainfrom
Omesh37:fix/skimage-rectangle-deprecated

Conversation

@Omesh37

@Omesh37 Omesh37 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Fixes a TypeError / FutureWarning caused by differing scikit-image APIs. The structuring element creation in pvanalytics/features/shading.py now uses the modern morphology.footprint_rectangle((1,3)) call.

Testing
image
image

  • Closes Tests emit FutureWarning from skimage #226
  • Non-API functions clearly documented with docstrings or comments as necessary.
  • Adds description and name entries in the appropriate "what's new" file
    in docs/whatsnew
    for all changes. Includes link to the GitHub Issue with :issue:`num`
    or this Pull Request with :pull:`num`. Includes contributor name
    and/or GitHub username (link with :ghuser:`user`).
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

@cwhanse cwhanse added the dependency Issues relating to dependencies or dependency management label Jul 3, 2026
@cwhanse cwhanse added this to the v0.2.3 milestone Jul 3, 2026
@cwhanse

cwhanse commented Jul 3, 2026

Copy link
Copy Markdown
Member

@Omesh37 did you test this locally? pvanalytics isn't getting much work and this fix has uncovered another unrelated problem.

@Omesh37

Omesh37 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@cwhanse Yes, I have tested using command pytest --cov=pvanalytics --cov-config=.coveragerc --cov-report term-missing pvanalytics --runslow locally, which results 224 passed, 29 skipped (require ruptures, not installed locally), 0 failed.
After appending .copy() at the end of ghi_image[...] = np.nan

@cwhanse

cwhanse commented Jul 5, 2026

Copy link
Copy Markdown
Member

@Omesh37 there's also an error related to converting a timedelta. If you don't have time/patience to track that down, I'll see if I can push to your PR.

@Omesh37

Omesh37 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@cwhanse I suggest a possible fix to this that is:

offset = frequencies.to_offset(freq)
try:
   return pd.to_timedelta(offset)
except ValueError:
   return pd.Timedelta(offset.nanos, unit='ns')

Storing pandas offset object and then returning the result. I have tested using:
pytest pvanalytics/tests/quality/test_gaps.py::test_completeness_score_no_data -v and it was successful locally.
But, by mistake i have pushed the code in wrong PR (#235)

@cwhanse

cwhanse commented Jul 6, 2026

Copy link
Copy Markdown
Member

@Omesh37 let's not fix that timedelta here. I'll submit a PR to fix it, then we can move this forward.

@Omesh37

Omesh37 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@cwhanse Sorry about that! I've reverted the timedelta change from PR #235. It now only contains the divide-by-zero fix in _fit.py.

@cwhanse

cwhanse commented Jul 7, 2026

Copy link
Copy Markdown
Member

@Omesh37 can you rebase this to main, so we can see if the Timedelta fix + this PR exposes any new errors? Thanks.

@Omesh37 Omesh37 force-pushed the fix/skimage-rectangle-deprecated branch from a9ff351 to cfd31c9 Compare July 8, 2026 03:40
@Omesh37

Omesh37 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@cwhanse I have rebased to main :)
Let's see what errors we get now!

@Omesh37

Omesh37 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@cwhanse The import fallback needed to be at module level rather than inside the function, otherwise footprint_rectangle was never bound before the call. All tests pass locally :)

Comment thread pvanalytics/features/shading.py Outdated
@cwhanse

cwhanse commented Jul 8, 2026

Copy link
Copy Markdown
Member

Le sigh. The fix I pushed in #236 requires pandas > 2.1. Since pvanalytics stops tests when the first one fails (I should change this), the error went undetected.

If you can find a way to fix that statement (series. index.diff()) without advancing pandas, that's fine. Or, I can bump up the minimum pandas version.

@Omesh37

Omesh37 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@cwhanse I tried using plain index subtraction which works across pandas versions.
seconds_per_sample = (series.index[1] - series.index[0]).total_seconds()
All the test was passed successfully :), could be a option

@cwhanse

cwhanse commented Jul 8, 2026

Copy link
Copy Markdown
Member

Winner. Thanks for sticking this one out.

@cwhanse cwhanse merged commit 06fd596 into pvlib:main Jul 8, 2026
30 checks passed
@Omesh37

Omesh37 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Finally it worked. Glad to help you out :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependency Issues relating to dependencies or dependency management

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tests emit FutureWarning from skimage

2 participants