vegandevs / vegan

R package for community ecologists: popular ordination methods, ecological null models & diversity analysis
https://vegandevs.github.io/vegan/
GNU General Public License v2.0
448 stars 97 forks source link

CLR: na.rm is not working #661

Open TuomasBorman opened 3 months ago

TuomasBorman commented 3 months ago

https://github.com/vegandevs/vegan/blob/69de595e4f47918ab7f893e9578647983cc3b3c5/R/decostand.R#L149

Hi,

I noticed that na.rm is not used in CLR calculation.

-Tuomas

jarioksa commented 3 months ago

There is a bigger problem than this. See my comments with #663. All these related standardizations share this same problem: users cannot set na.rm, but na.rm = TRUE will be always used (though with "clr" only after your suggested fix).

Here a dissection of the problem:

  1. decostand and vegdist have argument na.rm that defaults FALSE.
  2. method clr delegates calculations to .calc_clr, but it does not pass the value of na.rm set in decostand call. (It passes dot arguments, but na.rm is not among them.)
  3. .calc_clr sets na.rm = TRUE independent of settings in decostand or vegdist.
jarioksa commented 3 months ago

I am not sure about the internal logic of these functions. I need comment by @antagomir

Currently these methods work with hard-coded na.rm = TRUE in decostand. Looking at the code, it may be that this should be the case. Comments in .calc_rclr say so. If this is case, the calls to .calc_* functions should drop na.rm = TRUE from the function call and explicitly write na.rm = TRUE where needed. Only if there are cases where NA could optionally be retained, should we use na.rm = na.rm, with na.rm defined at the function call. Further, to make na.rm user settable, the .calc_* functions should be called as, say, .calc_clr(x, na.rm = na.rm, ...) where na.rm is set at the decostand call.

I started to make changes in these functions but got scared of fooling up, and I'll leave this till I hear from @antagomir .

antagomir commented 3 months ago

It would be better to activate na.rm for users. I can update this but will also check if we could finalize the matrix completion step to the rclr method. Will come back this week.

antagomir commented 3 months ago

@TuomasBorman kindly check #667 ?