ropensci / taxa

taxonomic classes for R
https://docs.ropensci.org/taxa
Other
48 stars 12 forks source link

merge binomen into this package #12

Closed sckott closed 7 years ago

sckott commented 7 years ago

from #6

sckott commented 7 years ago

i'm waiting on this until major changes are merged in in https://github.com/ropenscilabs/taxa/tree/issue-9

zachary-foster commented 7 years ago

Ok. The only thing that perhaps should get done is adding examples to the documentation for taxonomy, subtaxa, supertaxa, roots, and stems. Are you going to do that? I can also do it. Either way is fine with me. We can also merge now if you want; everything is working as far as I can tell.

sckott commented 7 years ago

Right on the examples, but most of your changes are not on master branch right? So I want to merge those in before merging together with binomen.

for examples happy to add some, but again, would be nice to merge in your changes first.

can you send PR soon?

zachary-foster commented 7 years ago

Oh yea, its ready to merge. I assumed you would make the changes on the issue-9 branch, but a PR makes more sense. I will send it now

sckott commented 7 years ago

thanks, i wasn't sure which branch was the most recent

sckott commented 7 years ago

i'm going to ask CRAN to archive binomen this week since we're rolling into this pkg, and it will fail on CRAN checks with a new dplyr coming out on 11 May

-- scratch that - forget that we were going to rename pkg to binomen - not sure if we'll have this pkg ready by 11 may, so i'll make a fix for new dplyr for binomen on cran

zachary-foster commented 7 years ago

Thanks for the update

sckott commented 7 years ago

working on branch include-binomen

sckott commented 7 years ago

@zachary-foster What do you think? See ?pop and ?hierarchy - thoughts on how that's implemented and how it behaves? good to decide on how we should structure these, then can roll out the rest of the functions following the same structure

sckott commented 7 years ago

oh, and install from include-binomen

zachary-foster commented 7 years ago

Cool, I will look at this later today

sckott commented 7 years ago

thx

zachary-foster commented 7 years ago

Looks good. Perhaps rename to something that mentioned rank? Like pop_rank? Or make usable with names and ids too?

It would be cool if we could do something like pop(data, species:family) too

When I was looking at this, I thought we might be able to add an abstracted version as well that can filter by taxon name and taxon ids or any other information we can derive from the class. We might be able to reuse a lot of code from taxmap to make an abstracted filtering function like filter_taxa for hierarchy. The basic idea would be:

For example:

filter_taxa(hier_obj, ranks == "species", ids %in% c(123, 456))

We could also add subtaxa and supertaxa options.

zachary-foster commented 7 years ago

Thinking about this more, the filter_taxa syntax could be apllied to all of the classes with multiple taxa like 'taxa' 'hierarchy' 'hierarchies' and 'taxonomy'. All of the NSE code can be reused and having a 'get_data' method for each class would make converting class info to tables a simple wrapper over get_data

sckott commented 7 years ago

Or make usable with names and ids too?

that sounds good, though we'll probably only be able to do a subset of operations with names and ids since there's no ordering to them

It would be cool if we could do something like pop(data, species:family) too

this is the operation strain in binomen, haven't ported that over yet

Will have a look at filter_taxa