[feature] diamond-square and MPD

[gottacatchenall]

A few things that need to be address before I go to much further

1. Diamond-Square can only be run on a matrix of size N by N where N=(2^n)+1 for some integer n. The two options would be to A) assert mat in _landscape!(mat, alg::DiamondSquare) is the right size, or B) run DS on the smallest NxN matrix that could contain mat and superimpose from the result onto mat. I lean toward A) because there's no hidden memory allocation behind the scenes

2. There is a way to implement DS recursively, but I think it is (somehow) even less elegant than the triple-loop at the moment. I'd be willing to give it a shot though

[codecov-io]

Codecov Report

Merging #19 (0265e21) into main (aece259) will decrease coverage by 6.39%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
- Coverage   16.39%   10.00%   -6.40%     
==========================================
  Files          10       11       +1     
  Lines         122      200      +78     
==========================================
  Hits           20       20              
- Misses        102      180      +78     

Flag	Coverage Δ
unittests	10.00% <0.00%> (-6.40%)	⬇️

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

Impacted Files	Coverage Δ
src/NeutralLandscapes.jl	100.00% <ø> (ø)
src/algorithms/diamondsquare.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 aece259...0265e21. Read the comment docs.

[mkborregaard]

Nice. Could allow B with a warning perhaps?

[tpoisot]

@gottacatchenall can you add an example to docs/src/gallery.md just to see how it looks?

[tpoisot]

Something that occurred to me when looking at mpd is that diamonsquare should probably be implemented as a low-level function before being a method, since it's used by mpd internally.

[gottacatchenall]

as far as i know, the only difference between mpd and diamond-square is that in mpd the midpoint of each edge is interpolated only from the corners of the square, as opposed to the corners and center of the square.

in this case, only diamond! would have to change to pass a different set of coordinates to interpolate(mat, coords::Vector{Tuple{Int,Int}})

i'm getting closer to a minimum functioning version, hopefully by tomorrow

[gottacatchenall]

this is functional, with the exception that the call rand!(zeros(65,65), DiamondSquare)) repeatedly works fine, but rand(DiamondSquare(), (65,65)) often (but not always) fails, giving a matrix usually full of NaNs, but sometimes values near 0 (e.g. 1e-55 everywhere). i cannot explain this atm

[gottacatchenall]

going to go through and add documentation later, and might try it clean up more

[mkborregaard]

a matrix usually full of NaNs, but sometimes values near 0 (e.g. 1e-55 everywhere).

Sounds like unassigned memory. Which is what rand generates here https://github.com/EcoJulia/NeutralLandscapes.jl/blob/main/src/landscape.jl#L33

When you use rand! in your example you instead initialise with a Matrix of 0's.

[gottacatchenall]

ah that makes sense, didn't realize rand didn't initialize zeros by default.

just pushed the documentation and am about to mark ready for review. at the moment the code is pretty verbose and not very "julia-ey" but seems to work fine

also i want to add the warning if mat is not the right size but could make that a new pr?

[mkborregaard]

Don't you think that the issue reveals a bug though? I can't quickly see it from your code, but I bet that it means that somewhere in your algo you look up a value in your matrix but accidentally look up an unassigned value? Maybe an off-by-1 error due to the one-based indexing? Just shooting from the hip.
I bet you could initialize your mat with just NaNs then check isfinite every time you read from mat to reveal if this happens.

[mkborregaard]

Most of these functions are internal and will never be exposed to users, right? Maybe just preface them with _ - and I don't think they need type signatures on arguments at all unless explicitly used for dispatch internally.

[tpoisot]

@gottacatchenall is this still wip or do you want a pre-merge review?

[gottacatchenall]

a couple of small changes i want to make before

• methods x(pt::Tuple{Int,Int}) = pt[1] and y(pt::Tuple{Int,Int}) = pt[2]to improve readability
• internal functions lead with _
• we should remove the @assert isfinite calls to improve speed right?

also i could implement mpd pretty easily, but could also do that in a separate pr

[gottacatchenall]

alright i think this is ready for a pre-merge review @tpoisot @mkborregaard

[tpoisot]

is this required?

[mkborregaard]

It makes a copy of mat if necessary and avoids it otherwise. Quite nice I think

[mkborregaard]

Nice 👍
Some rather minor comments

[gottacatchenall]

alright i think i got everything

[mkborregaard]

nice 👍

[mkborregaard]

sqaures->squares

[mkborregaard]

Should this maybe use the center rectangle rather than top-right one? Note sure if it makes a difference. @gottacatchenall