Skip to content

Fix #999: Replace hardcoded paths in Australia notebook with get_data and tempdir - #1418

Open
proofbyhuman wants to merge 4 commits into
weecology:mainfrom
proofbyhuman:fix-999-working-with-deepforest-data-docs
Open

Fix #999: Replace hardcoded paths in Australia notebook with get_data and tempdir#1418
proofbyhuman wants to merge 4 commits into
weecology:mainfrom
proofbyhuman:fix-999-working-with-deepforest-data-docs

Conversation

@proofbyhuman

Copy link
Copy Markdown

Description

This PR fixes #999 by removing hardcoded local user paths in the 'Working with deepforest data' documentation notebook (docs/user_guide/examples/Australia.ipynb).

Key changes:

  • Replaced hardcoded paths (/Users/benweinstein/Downloads/Plot13Ortho.tif) with get_data("australia.tif").
  • Replaced hardcoded output directories with cross-platform tempfile.gettempdir().
  • Added required imports (os and tempfile) to code blocks.

Related Issue(s)

Closes #999

AI-Assisted Development

  • I used AI tools (e.g., GitHub Copilot, ChatGPT, etc.) in developing this PR
  • I understand all the code I'm submitting
  • I have reviewed and validated all AI-generated code

AI tools used (if applicable):
Gemini (Google DeepMind Agentic Assistant)

proofbyhuman and others added 2 commits July 29, 2026 15:04
Fixes the nbqa-ruff pre-commit failure introduced in 874a2b8: the new
os/tempfile/get_data imports broke isort ordering. Groups them as
stdlib -> third-party -> first-party.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vickysharma-prog

Copy link
Copy Markdown
Contributor

Hi @proofbyhuman welcome to Deepforest, you have 10 PRs open (#1409#1418). I read all ten diffs and their linked issues. Consolidating into one comment instead of ten threads.

The set isn't mergeable as a set

Whichever merges first conflicts with the rest.

CI hasn't run

The green checks are only pre-commit.ci and readthedocs. The test suite is gated on maintainer approval, which you asked for on #1412. No test in any of these ten has executed. #1409 also states the suite wasn't run locally.

Per-PR

#1410 — duplicates existing code. ImagesCallback.on_train_start at src/deepforest/callbacks.py:53 already logs sample training images at train start, same default of 5 (dataset_samples=5). The existing one guards on distributed.is_global_zero(trainer) (yours has no rank guard — every DDP rank would log), skips on fast_dev_run, and samples the actual dataset via dataset[idx], i.e. post-augmentation. Yours re-reads the raw CSV, so it shows pre-augmentation annotations and won't catch the transform/coordinate bugs the description cites. It's also always-on rather than opt-in. Suggest closing.

#1409 — call sites not updated. 11 sites still pass csv_file= as a keyword and would emit DeprecationWarning: src/deepforest/scripts/predict.py:55 (production code), tests/test_evaluate.py:24,50,315,379, tests/test_IoU.py:14, tests/test_main.py:433,1169, tests/profile_evaluate.py:15, tests/profile_predict_file.py:17, docs/user_guide/12_evaluation.md:201. Two more pass positionally (tests/test_main.py:441, docs/user_guide/16_prediction.md:133) — those won't warn, they silently rebind to input_file.

Also: root_dir now defaults to None, but the docstring's "if none, uses image_dir in config" fallback isn't implemented, so None fails.

#1416 — orphans config.batch_size. After this change nothing reads it, but it stays in src/deepforest/conf/config.yaml:8 and src/deepforest/conf/schema.py:148. Anyone setting it is silently ignored, no warning. Needs removal or explicit deprecation. #889's item 3 ("update the config doc") also isn't covered. Minor: bird.yaml keeps batch_size: 32 alongside the new keys, and there's an unrelated lambda reformat in evaluate().

#1415 — wrong issue. Closes #1380 is incorrect. #1380 asks for a HuggingFace model card for weecology/cropmodel-deadtrees; this edits docs/user_guide/11_training.md. Separately, the rewritten example calls m.create_trainer(fast_dev_run=True) then m.trainer.fit(m) without configuring config.train.csv_file, so it fails as written.

#1414 — fragile detection. Catches bare Exception and substring-matches "401" / "Unauthorized". huggingface_hub raises typed GatedRepoError / HfHubHTTPError with .response.status_code — catch those and check == 401. Substring matching false-positives on any error text containing "401" (commit hash, path, port). The test patches with a plain Exception("401 Client Error..."), so it validates the substring match against its own mock, not real HF behavior. Also #1401 is a CI failure on a gated repo; a better message doesn't fix it, so this likely doesn't close the issue.

#1413 — network-dependent pin. Pins 0.80/0.72 against a live download of weecology/deepforest-tree. Fails for everyone the moment the model is republished, including on unrelated PRs. Deriving from integer counts (44/55, 44/61) is right, but consider pinning a revision or adding a marker so it can be deselected. Also uses csv_file=, deprecated by your #1409.

#1412 — removes a deliberate error. Deletes the workers > 0 ValueError in predict_tile and replaces it with a silent auto-override. That error looks intentional; turning a loud failure into a quiet correction is a maintainer decision. Test also calls m.load_model("weecology/deepforest-tree") — network-dependent and slow.

#1417 — matches #1274's spec. inverse_transform is identical to RandomPadTo's, worth sharing rather than duplicating. apply_transform returns a different shape than its input — fine at the default p=1.0, but you expose p, and if Kornia applies to a subset of a batch at p<1.0 the shapes may not concat. Worth testing with p=0.5.

#1418 — valid. australia.tif does ship in src/deepforest/data/. But the notebook's stored cell outputs weren't regenerated, so the rendered docs page still shows results from the old Plot13Ortho.tif. #999 is about the page rendering incorrectly, so re-run the notebook before committing.

#1411 — fine. Bare print() matches the surrounding code (datasets/training.py:71). callbacks.py uses pl_module.print() (rank-aware) but that isn't available in a dataset constructor.

On the AI-assisted workflow

Please review the Ai given code before submitting
You disclosed AI use on all ten via the template and named the tool each time — that's correct and not the issue. What's showing is that the review step is being skipped: #1411's description contains a stray proofbyhuman/antigravity string, #1410's ends with verification.ification, #1415 targets the wrong issue, and #1410 rebuilds something already in callbacks.py. Those are all catchable in a minute of reading. The "I have reviewed and validated all AI-generated code" checkbox is the load-bearing part, not the disclosure.

The specific gap is that these tools don't survey the existing codebase before adding to it — grepping for an existing implementation before starting would have caught #1410, and probably the #1409/#1413/#1416 overlap too.

Suggestion

Pick 2–3, draft the rest until those land. #1417, #1418 and #1409 look fine. That gives maintainers something reviewable and lets later PRs build on a merged base instead of conflicting.

@proofbyhuman

proofbyhuman commented Jul 30, 2026 via email

Copy link
Copy Markdown
Author

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.72%. Comparing base (d2851ec) to head (c07788e).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1418      +/-   ##
==========================================
+ Coverage   86.61%   86.72%   +0.11%     
==========================================
  Files          26       28       +2     
  Lines        3736     4069     +333     
==========================================
+ Hits         3236     3529     +293     
- Misses        500      540      +40     
Flag Coverage Δ
unittests 86.72% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

proofbyhuman and others added 2 commits July 30, 2026 18:21
…mage

The australia.tif bundled via get_data() is 708x751 pixels (much smaller
than the original ~15k x 14k image), so the crop slice [12000:13000]
was out of bounds. Changed to [200:500] which fits within the sample.

Also fixed the rasterio show() call: since r is now in channels-last
format (h, w, c), we need transpose(2, 0, 1) to convert to
channels-first (c, h, w) for rasterio, instead of the previous
np.rollaxis which assumed channels-first input.

Cleared stale outputs that contained hardcoded paths from the original
author's environment.
Fixes the nbqa-ruff pre-commit failure on e9ce8fc: replacing np.rollaxis
with r.transpose(2, 0, 1) removed the last use of numpy in the notebook,
leaving the import unused.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@proofbyhuman

Copy link
Copy Markdown
Author

Thanks for taking the time to go through all ten — one thread was far more useful than ten.

The process points are done: #1410 and #1414 are closed, and #1411, #1412, #1413, #1415 and #1416 are drafts. That leaves #1409, #1417 and #1418.

On re-running this notebook: I can't execute it as-is, and since the blocker is pre-existing code I'd rather ask than change something you didn't ask me to touch.

Cell 7 ends with:

plot = visualize.plot_results(results=boxes, image=r)
plt.imshow(plot)
plt.show()

plot_results returns matplotlib.figure.Figure (or Axes with axes=True), so plt.imshow raises TypeError: Image data of dtype object cannot be converted to float. Those three lines are unchanged from main — this PR doesn't touch them.

You're right that the outputs are the crux. To be precise about the current state: this branch cleared the stored outputs, because they embedded /Users/benweinstein/Downloads/... paths from the original run. With nbsphinx_execute = "never" in docs/conf.py, the rendered page now has no outputs at all rather than stale ones — which doesn't fix #999 either. So re-running is a hard requirement for this PR, not a nice-to-have; I just can't get there while cell 7 raises.

The minimal fix would be dropping the plt.imshow(plot) / plt.show() lines, since plot_results already draws with show=True. Would you prefer I do that here so the notebook can be executed and its outputs regenerated, or keep this PR to the path change and open a separate one for the execution fix?

One more thing I noticed while checking: australia.tif is 751x708 with 3 bands (EPSG:32753), not 4. The markdown in cells 3 and 5 walks through the four-band error and dropping the alpha channel, which was written for Plot13Ortho.tif and no longer matches what the code does. Let me know if you want that in scope too.

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.

Fix 'Working with deepforest data' docs page

2 participants