NEON-biodiversity / Ostats

O-statistics (community pairwise niche overlap statistics)
https://neon-biodiversity.github.io/Ostats/
Other
7 stars 2 forks source link

get_ses namespace issue #12

Closed sydnerecord closed 4 years ago

sydnerecord commented 4 years ago

Hi @qdread @yyue-r, In wrapping up my revisions on the man documentation for the Ostats package, I wasn't able to run the example for the get_ses function because the function could not be found by R. I'm wondering if it has something to do with how the namespace is functioning. get_ses currently sits in a file called 'utility_functions.R'. I'm wondering if something got screwed up between when Roxygen set the NAMESPACE document for the package and now. Any thoughts? Thanks! Sydne

qdread commented 4 years ago

Hey @sydnerecord @yyue-r . I did not include the function in the exported functions because it is an internal function that is only used within other functions. The user should never call it. Therefore, it really does not need to have any help documentation or examples. This is a typical practice for R package development: functions that are only used internally aren't exported and don't have documentation. Sorry if I didn't make that clear, which might have saved some work. I think this package already has a lot more documentation than a typical package would have, since the user is really not intended to call that many of the functions other than the main Ostats function and a couple of other ones.

So no, I don't think it's a screwup. It is working as intended. I don't think it is necessary to have an example in this case.

sydnerecord commented 4 years ago

Hi @qdread that makes sense. Do you know how we can modify the name space, so that it doesn't include those functions that are now wrapped into community_overlap_merged (in bold below)? @yyue-r please let me know if I'm missing any.

export(Ostat2longform) export(Ostats) export(Ostats_circular) export(Ostats_plot) export(Ostats_regional) export(circular_overlap) export(circular_overlap_24hour) export(community_overlap) export(community_overlap_circular) export(community_overlap_distributions) export(community_overlap_harmonicwmedian) export(community_overlap_merged) export(community_overlap_noitv) export(community_overlap_wm) export(community_overlap_wmedian) export(pairwise_overlap)

qdread commented 4 years ago

Well, I mean, if Arya and Isa already did a bunch of work in coming up with examples and documentation for those internal functions, we might as well keep them in the namespace at least for now.

However, I think that Arya and Isa's work of merging the different community_overlap_*() functions means that we can actually remove quite a few of those entirely (i.e., delete the source code from the package since they are now unused). I believe they have put all the functionality into community_overlap_merged(), correct @yyue-r @isafluck ? So for those unused variants, we can delete them from the source code (thanks to version control we will still be able to access that code if we need it in the future).

For the internal functions that are not going to be called by the user but that we still use in our package code, you would remove the @export line that appears right before the function definition in the source code. I am pretty sure that this will get rid of them from the namespace next time you run the document and build steps in devtools. But don't quote me on that.

isafluck commented 4 years ago

Hello! I remember that the get_ses example was not so functional/practical. So the idea of removing it from the functions the user can call sounds good. It would make the package simpler and I believe this is a positive point. If I'm not mistaken, Arya merged everything in the community_overlap_merged function (I remember she had a lot of work and the result was amazing), so yes, I believe all the functionality of the others community_overlap functions are into this merged function. @yyue-r can you confirm that?

sydnerecord commented 4 years ago

Hi @yyue-r, Can you take a look at this thread, especially Quentin's response? Please remove functions that won't be in the final package and work on the name space issues with @export.

yyue-r commented 4 years ago

Hi Sydne @sydnerecord I deleted the unnecessary functions and rebuilt the package. Now the functions are removed from namespace. However, I get a warning here when building: Warning message: In setup_ns_exports(path, export_all, export_imports) : Objects listed as exports, but not present in namespace: Ostats_circular, community_overlap, community_overlap_circular, community_overlap_distributions, community_overlap_harmonicwmedian, community_overlap_wm, community_overlap_wmedian

These functions need to be removed from the exports as well but I do not know how to do it.

qdread commented 4 years ago

@yyue-r I should be able to clear that issue up later tonight.

qdread commented 4 years ago

@yyue-r I rebuilt everything and it seems like you did it correctly. I did not get the warning. I think it's fine! I made a small unrelated correction to the vignette because the arguments to Ostats_plot had not been updated based on the changes from #13 . So I think all is good 😀 so I'm closing this.