Add rgb_range percentile support

[atanas-balevsky]

Add support for using percentiles instead of absolute values for the /rgb's endpoint *_range parameters.

gives us the possibility to request the /rgb?r_range=p2,p97... instead of calling /metadata first to fetch the pct's absolute values in order to construct an /rgb url

[dionhaefner]

Thanks for the PR.

There are some issues with it that look fixable. More importantly, I want at least one additional contributor to weigh in whether this is a good idea or not.

The interface is getting pretty overloaded, as evidenced by the type annotations:

ListOfRanges = Sequence[Optional[Tuple[Optional[NumberOrString], Optional[NumberOrString]]]]

Does anyone else have mixed feelings about this?

[dionhaefner]

This is not the correct format.

Previously, this accepted any number, which includes scientific notation like 1.3E-2. Also, percentiles must be ints between 0 and 100 and cannot be float numbers.

To prevent regressions I would advocate for still using fields.Number under the hood, with some additional logic to detect the percentile case. A custom field class should do, similar to what is described here.

[dionhaefner]

What does this do?

[atanas-balevsky]

Dear @dionhaefner ,
Thanks for the feedback! i've addressed some aspects of it.

now it's much clear that there can be a string or number. The regex is gone and there's more natural way of inferring the range type. i also added a test for the scientific notation when doing absolute values (didn't know you're supporting these)
i'm not sure why the tests action is not running now, but i believe it should be green

[mrpgraae]

Thanks for the PR.

There are some issues with it that look fixable. More importantly, I want at least one additional contributor to weigh in whether this is a good idea or not.

The interface is getting pretty overloaded, as evidenced by the type annotations:

ListOfRanges = Sequence[Optional[Tuple[Optional[NumberOrString], Optional[NumberOrString]]]]

Does anyone else have mixed feelings about this?

I may be biased since @atanas-balevsky is a colleague of mine, and we need this feature for something we are building over at SkyFi. Still, I think this is a good example of leveraging the fact that we have pre-computed information about an image to provide "magic" features like this, so I'd like to convince you that I think it's a good idea, even without my bias 🙂

Regarding the type, I think it was already quite complicated before. The only difference in this PR is that Number changed to NumberOrString. But if you think it would be better to do the percentiles as a separate query parameter, to not overload this parameter so much, then I don't see anything wrong with that. It would also help make the type validation simpler.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3cbc26c) 98.11% compared to head (63baa41) 98.15%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   98.11%   98.15%   +0.03%     
==========================================
  Files          52       53       +1     
  Lines        2338     2379      +41     
  Branches      476      486      +10     
==========================================
+ Hits         2294     2335      +41     
  Misses         29       29              
  Partials       15       15              

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

[dionhaefner]

Alright, I'm willing to merge this if we can address the issues above, plus:

1. Add full type annotations to all functions + arguments.
2. Ensure pre-commit hooks pass.
3. Add the same argument to /singleband.
4. Push coverage to the target.

Thanks @atanas-balevsky.

[atanas-balevsky]

Alright, I'm willing to merge this if we can address the issues above, plus:

1. Add full type annotations to all functions + arguments.
2. Ensure pre-commit hooks pass.
3. Add the same argument to /singleband.
4. Push coverage to the target.

Thanks @atanas-balevsky.

Dear @dionhaefner,
thanks for the feedback, i believe i addressed all the points. can we rerun the tests & reports?

Happy holidays!
Nas

[dionhaefner]

Thanks, this looks really good! Some final nits.

[atanas-balevsky]

Dear @dionhaefner!
Thanks for the feedback. i believe i addressed most of the comments. can we rerun the test actions?

[dionhaefner]

LGTM, thanks!