VincyaneBadouard / TreeData_broken

Harmonization and correction forest data tool.
https://vincyanebadouard.github.io/TreeData/
0 stars 1 forks source link

FullErrorProcessiong question(s) #60

Closed ValentineHerr closed 1 year ago

ValentineHerr commented 1 year ago

Hi @VincyaneBadouard,

In your function FullErrorProcessing, the options for argument WhatToCorrect are c("taper", "POM change", "punctual", "shift") (see this line) but in your code you do not allow taper (see this line)

Which of the two is correct?

VincyaneBadouard commented 1 year ago

Hi!

you can add "taper" to the line where it is missing

Sorry for the typo

ValentineHerr commented 1 year ago

great thanks!

ValentineHerr commented 1 year ago

@VincyaneBadouard, what happens if someone selects both taper and POM change?

ValentineHerr commented 1 year ago

General question for your @VincyaneBadouard, where can we tell if the arguments can be a multiple choice?

(I think I manually indicated if it was a multiple choice, but I can't remember how I knew)

ValentineHerr commented 1 year ago

@VincyaneBadouard the list of possible arguments here does not match the one here, which one is the correct one?

I think the correct is c("quadratic", "linear", "individual", "phylogenetic hierarchical") and I believe the user can select more than one. @VincyaneBadouard, please correct me if I am wrong

VincyaneBadouard commented 1 year ago

@VincyaneBadouard, what happens if someone selects both taper and POM change?

There will be the transformation of the diameters into a diameter equivalent to 1.30m (default value), then it will consider the initial POM and when there is a change of this initial POM it will make either an individual correction by weighted average of the diameter values that have been corrected by the taper, or a hierarchical phylogenetic correction (average of the growths of the species/genus/family, stand).

VincyaneBadouard commented 1 year ago

General question for your @VincyaneBadouard, where can we tell if the arguments can be a multiple choice?

(I think I manually indicated if it was a multiple choice, but I can't remember how I knew)

I'm not sur to understand your question. You tell it in the doc and in the check argument in the stop message. But it seems to me that you once found a function that automatically sends a message according to your criteria

VincyaneBadouard commented 1 year ago

@VincyaneBadouard the list of possible arguments here does not match the one here, which one is the correct one?

I think the correct is c("quadratic", "linear", "individual", "phylogenetic hierarchical") and I believe the user can select more than one. @VincyaneBadouard, please correct me if I am wrong

Yes sorry, it is not consistent. in fact after many modifications on the diametric correction methods, the quadratic regression is no longer possible because only 2 values are taken to make the regression (Local = TRUE). So there is no need to specify "linear" or "quadratic" in the current methods. On the other hand, indicating "individual" or "phylophylogenetic hierarchical" is essential, and if both are set, "individual" will be considered and the individual correction will be applied.

ValentineHerr commented 1 year ago

@VincyaneBadouard the list of possible arguments here does not match the one here, which one is the correct one?

I think the correct is c("quadratic", "linear", "individual", "phylogenetic hierarchical") and I believe the user can select more than one. @VincyaneBadouard, please correct me if I am wrong

Yes sorry, it is not consistent. in fact after many modifications on the diametric correction methods, the quadratic regression is no longer possible because only 2 values are taken to make the regression (Local = TRUE). So there is no need to specify "linear" or "quadratic" in the current methods. On the other hand, indicating "individual" or "phylophylogenetic hierarchical" is essential, and if both are set, "individual" will be considered and the individual correction will be applied.

Thanks for your answer. So I should remove "linear" in the example of DiameterCorrections, correct?

ValentineHerr commented 1 year ago

General question for your @VincyaneBadouard, where can we tell if the arguments can be a multiple choice? (I think I manually indicated if it was a multiple choice, but I can't remember how I knew)

I'm not sur to understand your question. You tell it in the doc and in the check argument in the stop message. But it seems to me that you once found a function that automatically sends a message according to your criteria

I meant if there was a clear indication somewhere (presumably in the doc), that the user can select multiplie options for one argument, for example run the function with WhatToCorrect = c("taper", "POM change", "punctual") or even WhatToCorrect = c("taper", "POM change", "punctual", "shift"), as oppose to only be allowed to pick one and only one of the options, like WhatToCorrect = "taper" or WhatToCorrect = "shift" I couldn't find a place in the doc that would specify that. (I have the same with CorrectionType)

ValentineHerr commented 1 year ago

Hi!

you can add "taper" to the line where it is missing

Sorry for the typo

@VincyaneBadouard I think I then also need to add a description for the option "taper" here. Would this be okay:

' @param WhatToCorrect Possible values: "taper", "POM change", "punctual", "shift"

' (character)

#' "taper"; transform the tree diameter measured at a given height into the diameter corresponding to the default measurement height (argument DefaultHOM), using an allometry (argument TaperFormula).

' - "POM change": detect POM change in the column POM and correct the

' Diameter values from it.

' - "punctual": detect if the error is punctual and correct it by

' interpolation.

' - "shift": detect if there is a shift of several Diameter values and

' links them to the 1st measurements set.

VincyaneBadouard commented 1 year ago

@VincyaneBadouard the list of possible arguments here does not match the one here, which one is the correct one?

I think the correct is c("quadratic", "linear", "individual", "phylogenetic hierarchical") and I believe the user can select more than one. @VincyaneBadouard, please correct me if I am wrong

Yes sorry, it is not consistent. in fact after many modifications on the diametric correction methods, the quadratic regression is no longer possible because only 2 values are taken to make the regression (Local = TRUE). So there is no need to specify "linear" or "quadratic" in the current methods. On the other hand, indicating "individual" or "phylophylogenetic hierarchical" is essential, and if both are set, "individual" will be considered and the individual correction will be applied.

Thanks for your answer. So I should remove "linear" in the example of DiameterCorrections, correct?

Yes thank you, again sorry

VincyaneBadouard commented 1 year ago

Hi! you can add "taper" to the line where it is missing Sorry for the typo

@VincyaneBadouard I think I then also need to add a description for the option "taper" here. Would this be okay:

' @param WhatToCorrect Possible values: "taper", "POM change", "punctual", "shift" #' (character) #' "taper"; transform the tree diameter measured at a given height into the diameter corresponding to the default measurement height (argument DefaultHOM), using an allometry (argument TaperFormula). #' - "POM change": detect POM change in the column POM and correct the #' Diameter values from it. #' - "punctual": detect if the error is punctual and correct it by #' interpolation. #' - "shift": detect if there is a shift of several Diameter values and #' links them to the 1st measurements set.

hmm no because it is not the DiameterCorrection function that does the taper correction. So only in the FullErrorProcessing function "taper" is accepted in the WhatToCorrect argument.

VincyaneBadouard commented 1 year ago

General question for your @VincyaneBadouard, where can we tell if the arguments can be a multiple choice? (I think I manually indicated if it was a multiple choice, but I can't remember how I knew)

I'm not sur to understand your question. You tell it in the doc and in the check argument in the stop message. But it seems to me that you once found a function that automatically sends a message according to your criteria

I meant if there was a clear indication somewhere (presumably in the doc), that the user can select multiplie options for one argument, for example run the function with WhatToCorrect = c("taper", "POM change", "punctual") or even WhatToCorrect = c("taper", "POM change", "punctual", "shift"), as oppose to only be allowed to pick one and only one of the options, like WhatToCorrect = "taper" or WhatToCorrect = "shift" I couldn't find a place in the doc that would specify that. (I have the same with CorrectionType)

I specify unless forgotten (which can be many) if only 1 value is accepted as here: https://github.com/VincyaneBadouard/TreeData/blob/3ff546d48ec47a022b7b66c924560b70cb4603e6/R/DiameterCorrection.R#L26

ValentineHerr commented 1 year ago

great thanks for your answers. I'll create a branch and a PR so you can double check the changes I'll make

VincyaneBadouard commented 1 year ago

Super, thank you for your checks! Have users tried the corrections?

ValentineHerr commented 1 year ago

no not yet, some of those issues (like the "taper" issue) are breaking the app, so I need to put more work into this.

ValentineHerr commented 1 year ago

hmm no because it is not the DiameterCorrection function that does the taper correction. So only in the FullErrorProcessing function "taper" is accepted in the WhatToCorrect argument.

I see now that FullErrorProcessing does not have the "taper" option in the help file because of #' @inheritParams DiameterCorrection

@VincyaneBadouard, Do you have a suggestion on how to add that in that help file?

VincyaneBadouard commented 1 year ago

No, sorry I don't know. Maybe the answer is in: https://cran.r-project.org/web/packages/roxygen2/vignettes/reuse.html

ValentineHerr commented 1 year ago

On the other hand, indicating "individual" or "phylophylogenetic hierarchical" is essential, and if both are set, "individual" will be considered and the individual correction will be applied.

This to me sounds like we should only allow one option, right? So in the help file we should add the "1 value" you were refering too, correct?.

#' @param CorrectionType Possible values: "individual", "phylogenetic
#'   hierarchical" (character, **1 value**).
ValentineHerr commented 1 year ago

No, sorry I don't know. Maybe the answer is in: https://cran.r-project.org/web/packages/roxygen2/vignettes/reuse.html

@VincyaneBadouard, tricky... Just to confirm, in Diameter Corrections, the use can select all 3 options "POM change", "punctual", "shift", correct? If he does, each correction is performed one after the other?

VincyaneBadouard commented 1 year ago

On the other hand, indicating "individual" or "phylophylogenetic hierarchical" is essential, and if both are set, "individual" will be considered and the individual correction will be applied.

This to me sounds like we should only allow one option, right? So in the help file we should add the "1 value" you were refering too, correct?.

#' @param CorrectionType Possible values: "individual", "phylogenetic
#'   hierarchical" (character, **1 value**).

absolutely

VincyaneBadouard commented 1 year ago

If he does, each correction is performed one after the other?

yes 1) POM change 2) detection if it's a shift or a punctual error 3) put NA for the punctual error 4) correct the shifts errors 5) replace NAs by regression

ValentineHerr commented 1 year ago

If he does, each correction is performed one after the other?

yes

  1. POM change
  2. detection if it's a shift or a punctual error
  3. put NA for the punctual error
  4. correct the shifts errors
  5. replace NAs by regression

Ok... maybe I'll create a separate "taper" argument in the FullErrorProcessing function