search

ropensci / tidync

NetCDF exploration and data extraction
https://docs.ropensci.org/tidync
90 stars 12 forks source link

Tests fail with latest RNetCDF #97

Closed mjwoods closed 2 years ago

mjwoods commented 5 years ago

When testing reverse dependencies of the latest RNetCDF release candidate (https://github.com/mjwoods/RNetCDF/releases/tag/v2.0-2), I found that the current CRAN version of tidync fails R CMD check. The checks also fail at the latest commit 540d6e72 of tidync, and the output log file testthat.Rout.fail contains the following error messages:

> library(testthat)
> library(tidync)
> 
> test_check("tidync")
── 1. Failure: utils work (@test-utils.R#51)  ──────────────────────────────────
`hv` not equal to hyper_vars(tnc).
Incompatible type for column `id`: x numeric, y integer
Incompatible type for column `ndims`: x numeric, y integer
Incompatible type for column `natts`: x numeric, y integer

── 2. Failure: utils work (@test-utils.R#52)  ──────────────────────────────────
`hd` not equal to hyper_dims(tnc).
Incompatible type for column `id`: x numeric, y integer

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 75 | SKIPPED: 5 | WARNINGS: 0 | FAILED: 2 ]
1. Failure: utils work (@test-utils.R#51) 
2. Failure: utils work (@test-utils.R#52) 

Error: testthat unit tests failed
Execution halted

These errors occur due to changes in the data types returned by var.inq.nc, which more closely match the types returned by the netcdf C library.

mdsumner commented 5 years ago

Oh dang sorry thought we had fixed that - but it is only testing expectations, leads me to to avoid those tests on CRAN in future

mjwoods commented 5 years ago

Sorry for the inconvenience, Michael. I regret changing the data types now, but it happened a long time ago and seemed like a sensible decision at the time. It does have the benefit of avoiding unnecessary type conversions between the NetCDF C library and R code, and the types are unlikely to change again without a major change to the NetCDF API (and none is planned, AFAIK).

It seems like a good idea to test the results from var.inq.nc in tidync. Perhaps you could coerce them to an expected type, so that the returned type does not matter. That way, the tests would pass for old and new versions of RNetCDF.

Would you mind if I submit the new RNetCDF package now? Or should I delay until you can submit an update of tidync?

mdsumner commented 5 years ago

No I don't mind, these revdep relationships always confuse me but thinking about it I realised there's no need to delay you - not at all, you should go ahead!

mjwoods commented 5 years ago

Thanks Michael, I have submitted the new RNetCDF to CRAN.

mjwoods commented 5 years ago

Hi Michael, is the change of data type in var.inq.nc likely to cause problems for users of tidync? That is, does it affect functionality of the package, apart from a minor failure in R CMD check?

mdsumner commented 5 years ago

It won't affect users, it's only my tests. You are making integer types where before we had double yeah? That's a good thing for sure! My tests would be better done with equivalence, it was too strict to test on exact type.

I'm pretty confident this will raise an error on CRAN, but it will be an easy fix with a few weeks grace and I'll take a closer look then. Good luck with CRAN!

mdsumner commented 5 years ago

Here it is, had meant to get onto this earlier - sorry CRAN ...

Please correct before 2019-10-25

https://cran.r-project.org/web/checks/check_results_tidync.html