Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issues #316, #317. #318

Merged
merged 1 commit into from
Sep 18, 2019
Merged

fix issues #316, #317. #318

merged 1 commit into from
Sep 18, 2019

Conversation

potterzot
Copy link
Contributor

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 sckott added this to the v0.9 milestone Sep 18, 2019
@sckott
Copy link
Contributor

sckott commented Sep 18, 2019

thanks @potterzot - LGTM
fix #316
fix #317

@sckott sckott merged commit 85df0d6 into ropensci:master Sep 18, 2019
@sckott
Copy link
Contributor

sckott commented Sep 18, 2019

@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:

  • For the test on line 30 i think you want %in% instead of ==. Also the test doesn't pass so not sure if it should be changed or what?
  • For the test on line 44, the left side results in an integer and the right side a numeric, so change the test to account for that. Possibly you changed something in this PR that changed the test result
  • For the test on line 58, the dimensions slot is NULL. i assume that's related to changes in this PR?

@potterzot
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants