zoonproject / zoon

The zoon R package
Other
61 stars 13 forks source link

change arguments to output modules #383

Open goldingn opened 7 years ago

goldingn commented 7 years ago

Output modules take two arguments, .model and .ras

This is confusing because:

  1. it seems unnecessary for df and model to be listed together
  2. .model is a confusing name for this list

I suggest we split .model$data up so that output modules take three default arguments: .df, .ras, .model

This would require changing both zoon::workflow(), various references in the vignettes, zoon's tests, and the existing output modules. The other functions, and the schematic in the zoon paper, should not be affected.

Can anyone think of a reason not to do this? cc @AugustT

AugustT commented 7 years ago

Seems fair to me. While you're there why not change the names? .df and .data are pretty much meaningless

timcdlucas commented 7 years ago

The only reason I can think of is that it might be best for all module types to have one default argument, a list of all the or bits. This saves space in the man page etc.

But probably your plan is better, split things into sensible chunks.

goldingn commented 7 years ago

@AugustT we discussed changing the names earlier, but couldn't think of anything better than df ras model. Open to suggestions though!

I kind of like the idea of making them all one standardised list. E.g. every module (or rather process, model & output modules) get the same first argument: .zoonInternal (or something), a list which always contains df, ras and/or model, depending on the module type.

so:

process modules would use .zoonInternals$df, .zoonInternals$ras model modules would use .zoonInternals$df output modules would use .zoonInternals$df, .zoonInternals$ras, .zoonInternals$model

thoughts?

timcdlucas commented 7 years ago

I think it simplifies things. The list name will be used a lot, so the shorter the better. Maybe .data for the whole list.