ropensci / taxa

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

Failing cran checks #104

Closed sckott closed 6 years ago

sckott commented 6 years ago

https://cranchecks.info/pkgs/taxa/

@zachary-foster got email from cran, we need to send a new version fixing these before Dec 19th

zachary-foster commented 6 years ago

Hi @sckott. Ok, sounds good. I want to release an update anyway.

sckott commented 6 years ago

okay, milestone v0.2 then.

I'll get to work on issues i'm responsible for

sckott commented 6 years ago

@zachary-foster is the issue list too long to get a version up by 19th next week https://github.com/ropensci/taxa/milestone/2 ? If so, can we come up with a slimmed down set that we think is doable.

zachary-foster commented 6 years ago

That might be a good idea. Most look pretty easy, but you can never tell until you start. I say we just start working through the easiest and put off what we don't get done until the next update. I don't think any are essential, besides fixing the CRAN checks. I am also working to get metacoder updated as well. I am hoping to coordinate the releases.

sckott commented 6 years ago

@zachary-foster should we set a goal of submitting by end of day today? like 5 pm or so, since all CRAN maintainers I believe are in Europe, so we should try to get this in before they get impatient

zachary-foster commented 6 years ago

Sure, I am ok with that, but I will probably not many of the new functionality done by then, but thats OK, it not essential. I think just focusing on going over the documentation and getting checks/tests to pass should be the main goal. Looking at the news.md, there are quite a few upgrades already since last version.

sckott commented 6 years ago

Okay. I do need to fix that tabular output problem, and make sure the checks will pass

sckott commented 6 years ago

Taxonomy is inherited in Taxmap, so need to make sure get_data_frame works there

zachary-foster commented 6 years ago

From how you have it set up, I think it will, since get_data works the same in taxonomy and taxmap, but most of the time, user-defined variables will not have a one-to-one relationship with taxa. It will still work if they only specify variables from the same table, but if they mix those with "taxon_names", for example, they will get an error.

It might be possible to make it work with outputs of different lengths, if we could match them up by taxon ID and only return the intersection of all taxon ID including duplications.

Probably a goal for a future version?

This function might be helpful for that:

intersect_with_dups <- function(a, b) {
        #taken from http://r.789695.n4.nabble.com/intersect-without-discarding-duplicates-td2225377.html
        rep(sort(intersect(a, b)), pmin(table(a[a %in% b]), table(b[b %in% a])))
 }
Reduce(intersect_with_dups, my_data)
sckott commented 6 years ago

That looks helpful. I've been hacking on a solution for taxmap objects as well, but not sure it will be very robust in the bit of time we have left. Okay to just have a stop message when variables are not of equal length? Or just remove get_data_frame for now until it works well for taxonomy and taxmap objects?

sckott commented 6 years ago

okay, updated get_data_frame, added simple tests for taxonomy and taxmap, and fixed example.

zachary-foster commented 6 years ago

Yea, I am fine with either just having it error when lengths are not equal or saving it for the next version. As it is, it is great for taxonomy objects, but has limited use with most of the dataset I use in taxmap objects, but there are definitely some tables I use in taxmap objects that are one-to-one with taxa by design that it would work well with.

It would be nice to be able to get it to the point that we can make a wrapper that allows saving a single table in a taxmap object with enough other information (mostly just the output of classifications) that it can be reconstructed using parse_tax_data. The function could even print out how to call parse_tax_data on its output. It could be a quick way for people to save their data in a csv and read it later when there is only one table. Thats a tangent though

sckott commented 6 years ago

checking the pkg on win builder and rhub

zachary-foster commented 6 years ago

Cool!

sckott commented 6 years ago

all is good with winbuilder - rhub failed on all, but unrelated to the pkg itself. popping over the git tag for v0.1 to see if I can see where those errors are coming from, but doesn't seem like it's happening anymore, so i guess its sorted

zachary-foster commented 6 years ago

Nice. I will go through the docs over the next hour and maybe fix that roots issue, but nothing that should break any checks

sckott commented 6 years ago

sounds good.

dropped into tag v0.1 and it seems the error is coming from this internal fxn https://github.com/ropensci/taxa/blob/master/R/taxonomy--class.R#L1001 - but the offending line https://github.com/ropensci/taxa/blob/v0.1.0/R/taxonomy--class.R#L924 has been reworked , so i imagine you saw this problems and already fixed

zachary-foster commented 6 years ago

Hmm, I vaguely remember reworking that. I think it is from this commit:

https://github.com/ropensci/taxa/commit/99c5e76ddb9c6e8ee19a7eec75c10a45d0613f67

sckott commented 6 years ago

looks like the library(metacoder) is causing problems https://travis-ci.org/ropensci/taxa/jobs/318341818#L1131

sckott commented 6 years ago

we'd have to list it in suggests if you want to keep it in the examples. if you put that example in readme, then we don't need to include it 😏

zachary-foster commented 6 years ago

Ok, that I will take it out for now. Eventually, I want to do a show a similar example highlighting all taxa that are roots, stems, etc, but thats probably better in the vignette anyway.

sckott commented 6 years ago

okay, are you doing that? or should I. we're ready to go unless you wanted to get this https://github.com/ropensci/taxa/issues/104#issuecomment-352591462 in before we push

zachary-foster commented 6 years ago

Yea, I just did locally as I am going through the docs

zachary-foster commented 6 years ago

Ok, I think I am done. I pushed my changes.

I get the following when I test

Warning message:
`encoding` is deprecated; all files now assumed to be UTF-8 

Is this a problem?

zachary-foster commented 6 years ago

R CMD check passes for me though

sckott commented 6 years ago

checking

sckott commented 6 years ago

i don't get that, looks good to me, will send off now

zachary-foster commented 6 years ago

Cool, thanks! I am glad to have this updated. Now I just need to get the metacoder update released that plays nice with taxa. I have been using the dev versions of taxa and metacoder for my research and it has been really nice to use. I also use taxa for a few side projects, including that field guide package I told you about, which is coming along nicely if slowly, so I have tested taxa a lot and it seems robust.

sckott commented 6 years ago

Nice, that's awesome you're using it so much. it will get better so much faster with the person working on it also using it.

okay to move all issues not dealt with in this milestone to v0.3 ?

zachary-foster commented 6 years ago

Yea, sounds good

sckott commented 6 years ago

ok