Module documentation shouldn't doc default module arguments

[timcdlucas]

Process modules must have the argument data. This shouldn't be documented (i.e. in the usage section) because then people will start trying to set that argument, when it is set by the internals of workflow.

[goldingn]

I'm not sure this is entirely true. We want people to roll their own modules too, so hiding those arguments would be confusing.

At present these argument are present in usage but not documented below.

I would suggest we add some placeholder text for these, along the lines:

data: fixed module input required by zoon

or similar.

This could easily be achieved by defining roxygen templates, and BuildModule adding the calls to these templates depending on the module type.

I think this is important to et right and should be low-hanging fruit so adding as a milestone.

[AugustT]

I agree these should really appear in the parameter list but be labelled in such a way that they are not confusing to beginner users of R.

One option is to include these at the end of the list of arguement:

Figshare <-
function (title, description = 'zoon workflow', authors = 'zoon',
               categories = 'SDM', tags='zoon', model, ras){

This makes them less visible to the user but contravenes the Google coding style.

Another option is to make it very clear in the parameter statement:

"model - Internal parameter, do not use in the workflow function. model is a model object e.g. of class glm or other returned by model modules. model is passed automatically in workflow from the model module(s) to the output module(s) and should not be passed by the user."

I like this second approach as it also makes it clear how the dataflow process works in workflow which could be useful for developers. If we agree I will start making these changes.

[timcdlucas]

I don't think we can put them at the end because many modules will have ... operators. There might be a way to work it, ggplot2 has dots at the beginning for example, but might be awkward.

Perhaps changing the internal arguments to .model, .ras etc. would make it very clear. And then make it clear in the docs as well.

[goldingn]

Agree they should stay at the front.

I like the idea of a different naming convention for the default arguments. I suspect users won't know that that .name is a convention for hidden objects, but think that doesn't matter so much.

[goldingn]

also @AugustT's argument doc is much better that what I suggested earlier

[AugustT]

I've been trying to think of a good reason not to change to the .argument format, primarily because it will mean quite a few changes to quite a few files.

I can't see any obvious reason why not to make this change, it doesn't seem to break any guidelines. using a leading fullstop will make the object invisible to ls() so it is worth thinking as to whether you think this will impact on the code anywhere.

Otherwise I think this is probably a good idea to move to .argument and documented parameters.

[goldingn]

yeah, I can't think where ls would be used in a module, but if it is it will probably be easy enough to change that in the module

[AugustT]

Okay, I will start making these changes to the module scripts and the workflow function. Let me know if you can think of any other places that changes might need to be made that I might not know of.

[timcdlucas]

Not a priority but the vignette https://github.com/zoonproject/zoon/blob/master/vignettes/interactive_zoon_usage.Rmd

will need changing.

Also Module_IO and Building_a_module here. I'm not even sure where those are originally written though as there is no .Rmd in vignettes/.

[AugustT]

@timcdlucas Can you help clarify something for me?

In the build module vignette the figures at the bottom (very handy!) say that the output module takes in a list(df and model) and a raster. However I cannot see anywhere in the code where df comes from since neither covariate modules nor model modules produce it and these are the sole inputs to output modules.

This is relevant to this issue because I'm trying to write the parameter descriptions for an output module.

[timcdlucas]

It's from ExtractAndCombine.

# combine with the occurrence data
df <- cbind(occurrence, occurrenceCovariates)

# Return as list of df and ras as required by process modules
return(list(df=df, ras=ras))

So it's the occurrence data with the environmental covariates at those points.

[AugustT]

Okay, but it also has a predictions column which is not produced by ExtractAndCombine.

I assume that there is a process after the model module in the workflow which takes df from ExtractAndCombine and uses the model to add predictions. This would make sense. If this is the case I suggest we add a little note to the 'build a module' figures (the last one) to tell developers where that data and predictions are coming from (I can't find the files for the vignette anywhere - any ideas?)

[timcdlucas]

Ah ok. Yes. df comes from RunModels which returns a list of a model object and the data.frame with the predictions column.

The section Cross and external validation in the interactive usage tries to explain it. But I think it might be wrong.

modelCrossvalid <- list(model = modCrossvalid$model, df = proc$df)

should be

modelCrossvalid <- list(model = modCrossvalid$model, df = modCrossvalid$data)

or something. Does that all sound right and make sense?

I can't remember where vignette files are supposed to go. But I think inst/doc/Building_a_module.Rmd is the raw source for that vignette. I'm not sure if it was supposed to be in /vignettes as well. And I'm not sure where the svg files for those pictures are supposed to go. They don't seem to be on github at all. I'll make a branch on my fork of zoon and put them in there.

[timcdlucas]

Those figures are now in https://github.com/timcdlucas/zoon/tree/addvignettepics/vignettes if you need them.

They were added in timcdlucas/zoon@0f7d3b57d7a4f4ffb190f8113dd3710167646e32 which I could probably PR request over if that was than saving them or something.

[goldingn]

SVG counts as code I reckon, so worth a PR?

[timcdlucas]

Pull request #115 add the SVGs.

[AugustT]

Hi @goldingn @timcdlucas,

I have been through and changed all of modules so that they are in the .arguement format and made the appropriate changes to the zoon package. I have also tested that every module still functions and have fixed a few issues in the process (those not fixed I have posted issues).

I have not yet re-written the BuildModule function (though I have done the minimum of renaming the default parameters). This should be amended so that the parameter description for the default arguments is automatically entered into the help files. I wanted to do this PR dispite this not being completed as I know the workshop is coming up and I wanted us to make sure testing is done on these changes.

Unfortunately I have had urgent work dropped on my desk today so I have limited time this week to make more changes. Happy to help out where I can though.

Tom

[goldingn]

Nice one, good work @AugustT!

I think we can leave BuildModule for the workshop given they will all be beginners and we still have a list of things to fix by then.

Definitely worth revisiting afterwards though

[goldingn]

158 is about fixing BuildModule, so I'm closing this