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

Attempt to resolve warnings in effect_N_discrete #97

Merged
merged 4 commits into from
Dec 2, 2016

Conversation

rueuntal
Copy link
Contributor

@rueuntal rueuntal commented Dec 2, 2016

This issue is described in #21 .

There were two warnings:

  • warning in rarefaction occurred when the summed N of two treatments were close. By swapping plot-level n among plots, the treatment with smaller N to start with would sometimes end up with even lower N, and as a result the individual-based curve couldn't be built all the way to the previous ending point.
  • warning in deltaS_N occurred when the treatments differed considerably in average density. The rarefaction curve of the higher-density-treatment could be shrunk below the ending point by the rescaling in deltaS_N.

In both cases I implemented the fix to fill the dataframe with NA's at the bottom, instead of rarefying beyond total N or extrapolating the rarefaction curve beyond its ending point.

warning('Extrapolating the rarefaction curve because the number of rescaled individuals is smaller than the inds argument')
interp_S_samp = pchip(c(1, rescaled_effort), c(1, S_samp), inds)
# No extrapolation of the rescaled rarefaction curve, only interpolation
interp_S_samp = pchip(c(1, rescaled_effort), c(1, S_samp), inds[inds <= max(rescaled_effort)])[1:length(inds)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job fixing this! I can see why this code works but it will difficult to understand for future us and other users. If we don't know that R adds NA's to cells that are longer than the vector is defined across we would be scratching our heads right now. I would prefer one step where you define interp_S_samp and then a second step where you add NA's to that vector so that it has the same length as inds. What do you think?

ind_sample_size)
effect_N_perm[, j] = group_effect_N_perm$deltaS
ind_sample_size[ind_sample_size <= sum(comm_level_perm)])
effect_N_perm[, j] = group_effect_N_perm$deltaS[1:nrow(effect_N_perm)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same basic feeling here as in the previous comment.

ind_sample_size)$deltaS)
null_N_deltaS_mat[i, ] = N_eff_perm[ , 2] - N_eff_perm[ , 1]
ind_sample_size[ind_sample_size <= min_N])$deltaS)
null_N_deltaS_mat[i, ] = (N_eff_perm[ , 2] - N_eff_perm[ , 1])[1:ncol(null_N_deltaS_mat)]
setTxtProgressBar(pb, k)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here, but great job diving in and tracking these problems down. I'll test your code out on the case-studies to see if anything changes drastically on the results.

@rueuntal
Copy link
Contributor Author

rueuntal commented Dec 2, 2016

Sure & done! (Except for the second case, which is already on its own line. But I did add a note to our future selves.)

@dmcglinn
Copy link
Member

dmcglinn commented Dec 2, 2016

Ok yea this isn't quite was I was thinking something more like x = c(x, rep(NA, length(inds) - length(x)) which makes it super clear what is going on but with the comments in the code this is pretty darn clear! Thanks for making that change!

@dmcglinn dmcglinn merged commit b345cb1 into MoBiodiv:master Dec 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants