classifyarray

[mkborregaard]

No description provided.

[mkborregaard]

there are two full copies in this function - it must be possible to avoid at least this one

[mkborregaard]

Just realized that calcboundaries is just quantile, though a little bit faster:

using StatsBase: quantile
a,c = rand(1000, 1000), [0.2, 0.4, 0.7, 0.8, 1]

julia> @btime quantile(vec(a), c)
  98.238 ms (5 allocations: 7.63 MiB)
5-element Vector{Float64}:
 0.20055273960046652
 0.40035575057383455
 0.7008467088240219
 0.8006453000863003
 0.9999981820198582

julia> @btime calcBoundaries(a, c)
  72.043 ms (7 allocations: 7.63 MiB)
5-element Vector{Float64}:
 0.2005524615247647
 0.40035544743228324
 0.7008461304132632
 0.8006452681104108
 0.9999981820198582

[tpoisot]

Isn't it because quantile is doing some checks internally that calcBoundaries doesn't? I'm more than willing to pay the 20ms penalty on a 10⁶ grid if the result is safer.

[mkborregaard]

Yes totally agree. It was just fun that it took me several readings of the code - and a port PR - to realize that this functionality already exists and has a name :-)

[mkborregaard]

But I just need to check out that quantile behaves as expected in the presences of NaNs

[tpoisot]

So, if calcboundaries is quantiles, shouldn't it be an overload of quantiles instead?

[codecov-io]

Codecov Report

Merging #10 (86cce3a) into main (19161a8) will decrease coverage by 7.43%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #10      +/-   ##
==========================================
- Coverage   37.73%   30.30%   -7.44%     
==========================================
  Files           8        9       +1     
  Lines          53       66      +13     
==========================================
  Hits           20       20              
- Misses         33       46      +13     

Flag	Coverage Δ
unittests	30.30% <0.00%> (-7.44%)	⬇️

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

Impacted Files	Coverage Δ
src/NeutralLandscapes.jl	100.00% <ø> (ø)
src/classify.jl	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19161a8...86cce3a. Read the comment docs.

[mkborregaard]

I think this is ready to be reviewed now - though there are neither docs nor tests.

a = rand(DistanceGradient(rand(1:50*50, 10)), 50, 50);
plot(
    heatmap(a), 
    heatmap(classifyArray!(copy(a), [0.25, 1, 1, 1, 0.25, 0.5])), 
    size = (800, 300)
)

[mkborregaard]

Honestly I feel like I should just inline those two helper functions? They're not used anywhere else.

[tpoisot]

Can you add an illustration to docs/src/gallery.md?

[tpoisot]

Honestly I feel like I should just inline those two helper functions? They're not used anywhere else.

I don't feel strongly about that - I might just rename them to start with _ to give a little hint that they're really not user-facing, but otherwise it doesn't really matter.

[mkborregaard]

Done. How de we feel about the camelCase? I'm almost inclined to just call it classify! though that sounds like something that could clash with other packages.

[tpoisot]

I like classify! - maybe we don't export it?

[mkborregaard]

Is NeutralLandscapes.classify! nicer than classifyLandscape!? 🤔

[mkborregaard]

Hi @tpoisot I had a number of issues with conflict - I think I resolved them, but I had to rebase and force-push - hope you wont' get into trouble over at #22 which you branched from this.
I renamed to classify! and unexported - if that works let's merge this now.
But actually as you can see in the example, the demolandscape function isn't actually that flexible, so it causes problems here - maybe it's better to go back to the raw code actually?

[rafaqz]

We could just export classify!. I can't find other uses of it, and people can use import to avoid it if they need to?

[rafaqz]

Looks good to me. I use BlueStyle most of the time now so the camelCase is a bit confusing in terms of separating out types and variables, but that's another one for the style guide.

[rafaqz]

could inline this or give a clearer name, took me a while to figure out what it was

[mkborregaard]

OK, I've exported it again (in the ecosystem only MLBase.jl appears to export a function of that name), and inline the two helper functions - and merged.

[tpoisot]

great, I'll be able to deal with #22 (which doesn't need it yet anyways, but the NN cluster does)

[tpoisot]

also in favor or not using demolandscape if it causes more issues than it solves