Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLR: na.rm is not working #661

Open
TuomasBorman opened this issue Jun 12, 2024 · 4 comments · May be fixed by #667
Open

CLR: na.rm is not working #661

TuomasBorman opened this issue Jun 12, 2024 · 4 comments · May be fixed by #667
Labels

Comments

@TuomasBorman
Copy link
Contributor

.calc_clr <-

Hi,

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

-Tuomas

@jarioksa
Copy link
Contributor

jarioksa commented Jun 24, 2024

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 jarioksa added the bug label Jun 24, 2024
@jarioksa
Copy link
Contributor

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
Copy link
Contributor

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 antagomir linked a pull request Jun 30, 2024 that will close this issue
@antagomir
Copy link
Contributor

@TuomasBorman kindly check #667 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants