Check peak count range

[swesterfeld]

Enforce sensible limits for --norm-peak-count and --raw-peak-count arguments for imagewmark get.

See #21, F05.

---

Summary by CodeRabbit

• Bug Fixes

  • Fixed peak selection to process all available peaks correctly.
  • Added validation to prevent invalid negative peak counts.

• Improvements

  • Peak count options now accept only integer values from 0 to 5,000, with clearer errors for out-of-range input.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 235fa0fd-093f-47b3-ad33-52fcb397f086

📥 Commits

Reviewing files that changed from the base of the PR and between 54d8730 and 9080a77.

📒 Files selected for processing (2)

• cxx/peaks2grid.cc
• src/imagewmark.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change constrains CLI peak counts to 0–5000. It also stops C++ peak selection when no candidates remain and asserts that parsed peak counts are non-negative.

Changes

Peak count handling

Layer / File(s)	Summary
CLI peak-count validation src/imagewmark.py	Adds int_range and applies it to --norm-peak-count and --raw-peak-count with inclusive bounds of 0–5000.
C++ peak-selection guards cxx/peaks2grid.cc	Stops selection when the peak vector is empty and asserts that parsed peak counts are non-negative.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: tim-janik

Merge Risk: ⚪ Minimal · up to 9080a

Peak-count inputs are bounded and selection stops safely when candidates run out. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 40.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: validating the peak count range.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch check-peak-count-range

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[tim-janik]

--norm-peak-count --raw-peak-count

Can you elaborate when/why people would actually want to set / use these directly?
Since it adds new user facing API, we also need a NEWS entry.

src/imagewmark.py: check that peak counts are in a useful range [0..5000]
See #21, F05.

Please paste the actual description / reasoning into the commit message, so we can make sense of commits offline without relying on github being up and still hosting ancient discussions.

[swesterfeld]

--norm-peak-count --raw-peak-count

Can you elaborate when/why people would actually want to set / use these directly? Since it adds new user facing API, we also need a NEWS entry.

This does not add new user facing API. The options were there before, and what is fixed by the PR is that you no longer can specify values like -1 or 2000000 because these don't make sense. So the idea of the PR is to make the existing options safer, not to add anything or remove anything.

Now if you want to discuss removing user facing API we already have, that is a different story.

First what it does (and why changing it could help): The options specify how many peaks (normalized and unnormalized) peaks2grid uses. Tweaking --norm-peak-count --raw-peak-count will make peaks2grid find different (better or worse) grids. I don't think more is better or less is better, but some reasonable middle value is best. Also too high peak counts make things slower. It probably also depends on the input what is best. But the more I think of it the less I think it is reasonable to document it and let the user to try and work with it if detecting a watermark for some image failed.

I see two options here:

• keep the peak count API but make it "secret / undocumented", so that I as developer still can use it for testing, but it doesn't get shown in --help (argparse supports this)
• remove the peak count API completely and hard code the 250 / 250 peak counts we currently have

Also we haven't systematically tested that 250 / 250 is really the best setting, so it may make sense to run a whole test suite with different peak counts and then possibly adjust the hard coded defaults.

src/imagewmark.py: check that peak counts are in a useful range [0..5000]
See #21, F05.

Please paste the actual description / reasoning into the commit message, so we can make sense of commits offline without relying on github being up and still hosting ancient discussions.

[tim-janik]

I see two options here:

* keep the peak count API but make it "secret / undocumented", so that I as developer still can use it for testing, but it doesn't get shown in --help (argparse supports this)

* remove the peak count API completely and hard code the 250 / 250 peak counts we currently have

Also we haven't systematically tested that 250 / 250 is really the best setting, so it may make sense to run a whole test suite with different peak counts and then possibly adjust the hard coded defaults.

Ok, then we should track that task. Please file a bug report with a description what exactly you recommend for testing.

[swesterfeld]

Ok, then we should track that task.

Makes sense, created new issue: #29.