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

add pkg_version & fix build/check errors #11

Merged
merged 4 commits into from
Apr 23, 2015

Conversation

aappling-usgs
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.95%) to 25.0% when pulling 7108e9b on aappling-usgs:master into 2a98cab on USGS-R:master.

@jordansread
Copy link

👍

jordansread pushed a commit that referenced this pull request Apr 23, 2015
add pkg_version & fix build/check errors
@jordansread jordansread merged commit 1742276 into DOI-USGS:master Apr 23, 2015
@@ -24,7 +24,9 @@
#'
#' @author Alison Appling, Jordan Read; modeled on lakeMetabolizer
#' @examples
#' metab_simple(data=data.frame(date.time=rep(as.Date("2015-04-15"), 24), DO.obs=1:24, DO.sat=rep(14, 24), PAR=sin((1:24)*pi/24)^8, temp.water=15))
#' metab_simple(data=data.frame(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking about this basic structure. Should we skip the input for DO.sat and calculate that internally? Seems a bit redundant w/ DO.obs, given we recommend it gets passed through o2.at.sat anyhow to build DO.sat. Also, could support additional args to o2.at.sat with the .... Just a thought. I see some pros and cons either way.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do.sat was passed in for LakeMetabolizer because the data and metadata requirements to calculate it include information you might not normally pass in. Model choice, elevation or barometric pressure. Making people calculate it ahead of time adds tasks for the user but reduces data inputs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but a lot of use cases assume salinity~0, and use altitude for baro, so since wtr.temp and do.obs are already there, it doesn't seem onerous to support

metab_simple(data = data.frame(date.time=rep(as.Date("2015-04-15"), 24), DO.obs=1:24, temp.water=15), altitude=424)
# as input to 
metab_simple(data, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a separate function that does all the data preparation for you?
Then we can also (later) create a high-level function that both prepares
and models the data, but our core modeling functions will remain relatively
short and focused on a single activity (fitting a model).

On Thu, Apr 23, 2015 at 7:54 AM, Jordan S Read notifications@github.com
wrote:

In R/metab_simple.R
#11 (comment)
:

@@ -24,7 +24,9 @@
#'
#' @author Alison Appling, Jordan Read; modeled on lakeMetabolizer
#' @examples
-#' metab_simple(data=data.frame(date.time=rep(as.Date("2015-04-15"), 24), DO.obs=1:24, DO.sat=rep(14, 24), PAR=sin((1:24)*pi/24)^8, temp.water=15))
+#' metab_simple(data=data.frame(

Ok, but a lot of use cases assume salinity~0, and use altitude for baro,
so since wtr.temp and do.obs are already there, it doesn't seem onerous to
support

metab_simple(data = data.frame(date.time=rep(as.Date("2015-04-15"), 24), DO.obs=1:24, temp.water=15), altitude=424)# as input to
metab_simple(data, ...)


Reply to this email directly or view it on GitHub
https://github.com/USGS-R/streamMetabolizer/pull/11/files#r28959102.

Alison Appling
Postdoctoral Fellow, USGS Powell Center
USGS Center for Integrated Data Analytics and University of Wisconsin -
Madison
Phone: (608) 821-3927

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, was just laying out the thought process for both.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lawinslow we are going to do the low level stuff and do conversions elsewhere. Ignore my early morning musings :)

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.

None yet

4 participants