search

bcgov / rcaaqs

An R package to facilitate the calculation of air quality metrics according to Canadian Ambient Air Quality Standards
Apache License 2.0
16 stars 5 forks source link

Completed pull request (ready for merge) #17

Closed nograpes closed 7 years ago

nograpes commented 7 years ago

In this initial set of commits, I have created some generic "workhorse" functions in window.R, and then used these generic functions to implement the ozone calculations requested in issue #2 in a set of 4 functions (o3_daily_max, o3_rolling_8hr_avg, o3_ann_4th_highest, o3_three_yr_avg).

Next steps will be to implement the other 3 functions mentioned in issue #11, #12, #13, using these generic functions.

ateucher commented 7 years ago

Thanks @nograpes! I'll have a look and get back to you. But feel free to keep working while I do.

ateucher commented 7 years ago

This is looking really good @nograpes! I think you addressed all of my original comments. I've started poking around a bit again but won't do a full review until you let me know you're ready

nograpes commented 7 years ago

Great, I still have more than a few tests to add, but I'm getting there. It'll be ready for review by midnight tonight.

nograpes commented 7 years ago

It is now ready for a full review.

nograpes commented 7 years ago

All the changes have been made, but unfortunately travis seems to be failing when building against the dev version of R. It can't seem to find all the dependencies like rgdal and sp. I'm working through the problem now.

ateucher commented 7 years ago

Getting gdal and friends working in Travis can be a nightmare, but I have some experience with it. I'll have a look too

ateucher commented 7 years ago

Also, if it's building fine on r-release you can just remove the R-devel line in .travis.yml and I can fix it later

nograpes commented 7 years ago

Okay, I'll remove the dev version now, but I'll continue to debug the issue

ateucher commented 7 years ago

@nograpes once you have fixed that one bug and added some tests for the exclusion functionality, I think we will be good to merge.

nograpes commented 7 years ago

Okay, so that last round of checking and new tests revealed a bug and few annoying inconsistencies. Everything is all sorted out now, and I am declaring this ready to merge.

ateucher commented 7 years ago

Nice catches - adding tests is great for that! Happens to me all the time :) LGTM!