search

ropensci / taxa

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

`taxonomy` questions #10

Closed zachary-foster closed 7 years ago

zachary-foster commented 7 years ago

Hi @sckott, nice work with the taxonomy class! I am looking through the code and trying to figure a few things out:

  1. Whats going on here? I dont see a taxa variable. It results in no. hierarchies: 0 when the print method is used.
  2. Do we need to store the edgelists and graph variables? They seem redundant to me and would require recalculation every time a change is made. make_graph could be used in the print method directly instead of graph. The contents of edgelists could be inferred from edgelist, except for multiple instances of the same edge list, but if users are interested in that they should use hierarchies or taxmap (with a column for taxon counts).
  3. How about calling uniqtaxa taxa or changing the private function unique_taxa to something like get_unique_taxa, so that the public variable can be unique_taxa?

I am going to play with it on a new branch and submit a PR if I come up with anything good.

sckott commented 7 years ago

fixed no. 1 above, sorry, had originally assigned input hierarchies to a variable called taxa, but then removed it.

sckott commented 7 years ago

No, probably don't need to store edgelists and graph variables

I changed the internal method unique_taxa to get_unique_taxa, but didn't change uniqtaxa, but you can change it

zachary-foster commented 7 years ago

Ok, sounds good. I will pull your changes and merge them with mine before submitting a PR.

zachary-foster commented 7 years ago

It seems like this has been addressed by the recent PR

sckott commented 7 years ago

agree