refeactor and docs #49

rafaqz · 2021-03-03T13:04:16Z

What the pull request does
Refactors and cleans up a little, standardises all algorithm arguments and docs, and fixes documentation so there are no warnings - which means documenting everything exported, and moving a lot of docs to code comments.

Type of change
Please indicate the relevant option(s)

🐛 Bug fix (non-breaking change which fixes an issue)
❇️ New feature (non-breaking change which adds functionality)
💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
📖 This change requires a documentation update

Checklist

The changes are documented
- The docstrings of the different functions describe the arguments, purpose, and behavior
- There are examples in the documentation website
The changes are tested
The changes do not modify the behavior of the previously existing functions
- If they do, please explain why and how in the introduction paragraph
For new contributors - my name and information are added to .zenodo.json
The Project.toml field version has been updated
- Change the last number for a v0.0.x series release, a bug fix, or a performance improvement
- Change the middle number for additional features that do not break the current test suite (unless you fix a bug in the test suite)
- Change the first number for changes that break the current test suite

codecov-io · 2021-03-03T13:06:25Z

Codecov Report

Merging #49 (cecab92) into main (18df1db) will decrease coverage by 0.74%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
- Coverage   62.45%   61.71%   -0.75%     
==========================================
  Files          14       14              
  Lines         277      269       -8     
==========================================
- Hits          173      166       -7     
+ Misses        104      103       -1

Flag	Coverage Δ
unittests	`61.71% <50.00%> (-0.75%)`	⬇️

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/discretevoronoi.jl	`0.00% <0.00%> (ø)`
src/algorithms/distancegradient.jl	`0.00% <ø> (ø)`
src/algorithms/edgegradient.jl	`100.00% <ø> (ø)`
src/algorithms/nncluster.jl	`0.00% <0.00%> (ø)`
src/algorithms/nnelement.jl	`94.44% <ø> (-0.30%)`	⬇️
src/algorithms/nogradient.jl	`100.00% <ø> (ø)`
src/algorithms/perlinnoise.jl	`89.65% <ø> (ø)`
src/algorithms/planargradient.jl	`100.00% <ø> (ø)`
src/algorithms/rectangularcluster.jl	`100.00% <ø> (ø)`
... and 5 more

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 18df1db...cecab92. Read the comment docs.

mkborregaard

Looks sensible - some few comments on naming. This only covers some of the stuff discussed at #47 right?

mkborregaard · 2021-03-03T13:28:25Z

src/NeutralLandscapes.jl


-include(joinpath("algorithms", "diamondsquare.jl"))
+export DiscreteVoronoi
 export DiamondSquare, MidpointDisplacement


good idea to give mpd it's own file

I think it's mostly an application of DiamondSquare, but not opposed to the idea - @gottacatchenall ?

mkborregaard · 2021-03-03T13:30:02Z

src/algorithms/diamondsquare.jl

@@ -21,48 +23,40 @@ edge midpoints from the nearest two corners and the square's center, where as
 midpoint-displacement only intepolates from the nearest corners (see `MidpointDisplacement`).

 """
-struct DiamondSquare <: NeutralLandscapeMaker
-    H::Float64
+@kwdef struct DiamondSquare <: NeutralLandscapeMaker


I don't usually use @kwdef - does that mean you need to call it as DiamondSquare(H = 0.2) to initialize this object? If that's the case I suggest replacing H with a more informative variable name like autocor or even autocorrelation (and the same for all other algs)

I think all packages use H, so I would document what it means, and then use the same syntax for constructors as the other methods. This would make the code more consistent.

@kwdef is basically a shortcut for adding keyword constructors. The single argument methods don't really need them, but they're nice for multi-argument methods, so I standardised that all have both keywords and regular arg constructors to avoid confusion.

You can still use regular argument constructors - I just forgot to include that in this particular doc, it's in most of the other docs.

And yes the fieldnames as keywords do become more important. I'm not a huge fan of math in struct fields, it tends to make the algorithms impenetrable. H = alg.autocor is my usual preference. But if it's common in the package it's probably ok.

mkborregaard · 2021-03-03T13:31:16Z

src/algorithms/diamondsquare.jl

+Creates a midpoint-displacement algorithm object `MidpointDisplacement`. 
+The degree of spatial autocorrelation is controlled by a parameter `H`,
+which varies from 0.0 (low autocorrelation) to 1.0 (high autocorrelation) --- 
+note this is non-inclusive and H = 0 and H = 1 will not behavive as expected.


behavive => behave

mkborregaard · 2021-03-03T13:32:02Z

src/algorithms/diamondsquare.jl

+A similar algorithm, diamond-square, functions almost
+identically, except that in diamond-square, the square step interpolates
+edge midpoints from the nearest two corners and the square's center, where as 
+`MidpointDisplacement` only intepolates from the nearest corners (see `DiamondSquare`).


intepolates => interpolates

mkborregaard · 2021-03-03T13:33:03Z

src/algorithms/diamondsquare.jl

-
-    Computes the mean of a set of points, represented as a list of indecies to a matrix `mat`.
-"""
+# Computes the mean of a set of points, represented as a list of indecies to a matrix `mat`.


indecies => indices

tpoisot

bring the documentation from internal functions into dosctrings as opposed to into comments
do we want to use @kwdef?

tpoisot · 2021-03-03T16:14:27Z

src/NeutralLandscapes.jl


-include(joinpath("algorithms", "diamondsquare.jl"))
+export DiscreteVoronoi
 export DiamondSquare, MidpointDisplacement


I think it's mostly an application of DiamondSquare, but not opposed to the idea - @gottacatchenall ?

tpoisot · 2021-03-03T16:15:00Z

src/NeutralLandscapes.jl

 using NearestNeighbors: KDTree, knn, nn
 using DataStructures: IntDisjointSets, union!, find_root, push!
+using Base: @kwdef


As far as I can tell it's only used by a single type - can't we just remove it?

@kwdef is used on all types now

👍 let's keep it

tpoisot · 2021-03-03T16:16:17Z

src/algorithms/diamondsquare.jl

@@ -21,48 +23,40 @@ edge midpoints from the nearest two corners and the square's center, where as
 midpoint-displacement only intepolates from the nearest corners (see `MidpointDisplacement`).

 """
-struct DiamondSquare <: NeutralLandscapeMaker
-    H::Float64
+@kwdef struct DiamondSquare <: NeutralLandscapeMaker


I think all packages use H, so I would document what it means, and then use the same syntax for constructors as the other methods. This would make the code more consistent.

tpoisot · 2021-03-03T16:17:08Z

src/algorithms/diamondsquare.jl

-"""
+# Check if `mat` is the right size.
+# If mat is not the correct size (DiamondSquare can only run on a lattice of size NxN where N = (2^n)+1 for integer n),
+# allocates the smallest lattice large enough to contain `mat` that can run DiamondSquare.


Why is this in comments instead of a docstring? I think the docstring versions makes more sense.

Docstrings not in docs throw a warning that will be a test failure

tpoisot · 2021-03-03T16:17:19Z

src/algorithms/diamondsquare.jl


-"""
+# Diamond-square is an iterative procedure, where the lattice is divided


same as above

tpoisot · 2021-03-03T16:17:25Z

src/algorithms/diamondsquare.jl

-    Initialize's the `DiamondSquare` algorithm by displacing the four corners of the
-    lattice using `displace`, scaled by the algorithm's autocorrelation `H`.
-"""
+# Initialize's the `DiamondSquare` algorithm by displacing the four corners of the


same as above

tpoisot · 2021-03-03T16:18:15Z

src/algorithms/edgegradient.jl


 This type is used to generate an edge gradient landscape, where values change
 as a bilinear function of the *x* and *y* coordinates. The direction is
 expressed as a floating point value, which will be in *[0,360]*. The inner
 constructor takes the mod of the value passed and 360, so that a value that is
 out of the correct interval will be corrected.
 """
-struct EdgeGradient <: NeutralLandscapeMaker
-    direction::Float64
+@kwdef struct EdgeGradient <: NeutralLandscapeMaker


I would really rather get rid of @kwdef - it doesn't seem the be the "community standard" for constructors.

That's largely because Parameters.jl predates it so there isn't really a community standard. I even have another package that does this differently! (FieldDefaults.jl).

Out options using Base are to use @kwdef or to write out the keyword argument constructors - are you saying you would prefer them fully written out or not to have keyword constructors?

mkborregaard · 2021-03-03T18:37:17Z

But why do we want docstrings on unexported functions that are never meant to be user-facing?

tpoisot · 2021-03-03T18:46:53Z

But why do we want docstrings on unexported functions that are never meant to be user-facing?

Maybe that's just me, but I like having the docstring when I'm writing things for the package - as in, when I hover the function name in VS Code - it's easier than having to look into the code for the comments.

mkborregaard · 2021-03-03T19:48:57Z

It seems a little strange to me, but I don't feel strongly about it, as long as they are kept out of the docs :-)

rafaqz · 2021-03-03T21:13:51Z

If you have docstrings not in the docs, you get a warning so tests wont pass when strict=true. If you turn off strict to fix this, they will pass when a new PR misses a docstring or messes something up.

The idea was to enforce documentation. Its made my life a lot easier on my own packages to get test failures whenever the docs break in any way, and made the docs more reliable.

Otherwise Im easy either way. But underscore functions are for devs only, so comments seem right to me anyway?

And why not use @kwdef? I guess we can write the constructors out. But it is a base macro.

tpoisot · 2021-03-03T21:35:26Z

Let's use @kwdef if it makes it user frieldier. I still think internal functions should be documented - a thing that many packages do is to have a specific page for them called "internals" or something. It can start with a disclaimer that people should probably not use these functions for daily use.

mkborregaard · 2021-03-04T07:35:55Z

I'm OK with having them under "internals" if that is useful for you guys. And I don't care either way about kwdef :-) Then we just have some docstrings to write - I didn't docstring any internal functions.

tpoisot · 2021-03-06T23:37:32Z

I'm fine with the package as is, so I'm going to merge and tag as soon as it passes. I'll start building the SimpleSDMLayers integration as soon as we have a version!

rafaqz changed the title ~~Rs/docs~~ refeactor and docs Mar 3, 2021

rafaqz requested review from tpoisot, mkborregaard and gottacatchenall and removed request for tpoisot and mkborregaard March 3, 2021 13:10

mkborregaard reviewed Mar 3, 2021

View reviewed changes

tpoisot suggested changes Mar 3, 2021

View reviewed changes

tpoisot approved these changes Mar 6, 2021

View reviewed changes

tpoisot removed the request for review from gottacatchenall March 6, 2021 23:37

rafaqz added 5 commits March 7, 2021 13:31

force checkdocs=:all and strict=true

aa67702

standardise keywords and doc strings for algorithms

1f40a96

fix index.md

1f9fd8f

refactor and fix docs

732bf3e

fix alg docs so all have both kw and arg forms

2d33236

rafaqz force-pushed the rs/docs branch from 8a6b86a to 2d33236 Compare March 7, 2021 02:40

fix typos

445a793

tpoisot merged commit 478a2b4 into main Mar 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refeactor and docs #49

refeactor and docs #49

rafaqz commented Mar 3, 2021 •

edited

Loading

codecov-io commented Mar 3, 2021 •

edited

Loading

mkborregaard left a comment

mkborregaard Mar 3, 2021

tpoisot Mar 3, 2021

mkborregaard Mar 3, 2021

tpoisot Mar 3, 2021

rafaqz Mar 3, 2021

mkborregaard Mar 3, 2021

mkborregaard Mar 3, 2021

mkborregaard Mar 3, 2021

tpoisot left a comment

tpoisot Mar 3, 2021

tpoisot Mar 3, 2021

rafaqz Mar 3, 2021

tpoisot Mar 6, 2021

tpoisot Mar 3, 2021

tpoisot Mar 3, 2021

rafaqz Mar 3, 2021

tpoisot Mar 3, 2021

tpoisot Mar 3, 2021

tpoisot Mar 3, 2021

rafaqz Mar 3, 2021 •

edited

Loading

mkborregaard commented Mar 3, 2021

tpoisot commented Mar 3, 2021

mkborregaard commented Mar 3, 2021

rafaqz commented Mar 3, 2021 •

edited

Loading

tpoisot commented Mar 3, 2021

mkborregaard commented Mar 4, 2021 •

edited

Loading

tpoisot commented Mar 6, 2021


		"""
		# Diamond-square is an iterative procedure, where the lattice is divided

refeactor and docs #49

refeactor and docs #49

Conversation

rafaqz commented Mar 3, 2021 • edited Loading

codecov-io commented Mar 3, 2021 • edited Loading

Codecov Report

mkborregaard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpoisot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

mkborregaard commented Mar 3, 2021

tpoisot commented Mar 3, 2021

mkborregaard commented Mar 3, 2021

rafaqz commented Mar 3, 2021 • edited Loading

tpoisot commented Mar 3, 2021

mkborregaard commented Mar 4, 2021 • edited Loading

tpoisot commented Mar 6, 2021

rafaqz commented Mar 3, 2021 •

edited

Loading

codecov-io commented Mar 3, 2021 •

edited

Loading

rafaqz Mar 3, 2021 •

edited

Loading

rafaqz commented Mar 3, 2021 •

edited

Loading

mkborregaard commented Mar 4, 2021 •

edited

Loading