ropensci / rnoaa

R interface to many NOAA data APIs
https://docs.ropensci.org/rnoaa
Other
330 stars 84 forks source link

fix issues #316, #317. #318

Closed potterzot closed 5 years ago

potterzot commented 5 years ago

Description

This addresses issue #316 and also makes a quick fix to #317. The primary issue was that the longitude coordinates were not correctly being handled in the conversion from the -180 - 180 coordinate system to the 0 - 360 system. The program was taking the max() when it should have been taking the min(). However, on review there was a much simpler way of handling the indexing that more closely linked directly to values. The result is both easier to read and less error prone.

This PR also includes a new test to check the latitude and longitude values are those queried.

sckott commented 5 years ago

thanks @potterzot - LGTM fix #316 fix #317

sckott commented 5 years ago

@potterzot i missed that the tests don't actually pass, did they pass for you or did you not run them on your end?

test-gefs.R:30: warning: gefs latitude and longitude selection returns correct values
longer object length is not a multiple of shorter object length

test-gefs.R:30: failure: gefs latitude and longitude selection returns correct values
all(unique(a$data$lat) == c(54, 54, 54, 54)) isn't true.

test-gefs.R:44: failure: gefs time and ensemble selection returns correct indices.
unique(d$data$ens) not equal to ens_idx - 1.
Attributes: < Modes: list, NULL >
Attributes: < Lengths: 1, 0 >
Attributes: < names for target but not for current >
Attributes: < current is not list-like >

test-gefs.R:58: failure: gefs metadata
d$dimensions[1:4] not equal to c("lon", "lat", "height_above_ground", "ens").
target is NULL, current is character

Apologies I should have looked more closely at this PR. Can you send a new PR? Some suggestions:

potterzot commented 5 years ago

Ack sorry, they passed on my end but I must have done something after that but before the commit. Sorry! I'll look into it asap.