GLEON / LakeMetabolizer

Collection of Lake Metabolism Functions
16 stars 10 forks source link

Time change seems to be causing issues #130

Open nrlottig opened 6 years ago

nrlottig commented 6 years ago

I'm getting the following error running k.read.base:

Error in data.frame(dateTime = dat$dateTime, C_D = C_D, alh = alh, ash = ash) : arguments imply differing number of rows: 16035, 16036

I believe that I have tracked it down to calc.zeng line 20. My dataframe includes the following data

2017-03-12 02:00:00 which returns an NA for line 20 which then shortens the dataframe to 16035 rows. There also seem to be a whole bunch of errors that cascade off of that length mismatch (e.g., line 53 atm.press is 16036 while e_a(line 52) is 16035.

nrlottig commented 6 years ago

I just deleted that single row from my dataset and everything ran fine.

jzwart commented 6 years ago

Hey Noah, I think this is a daylight savings issue. 2017-03-12 from 02:00:00 to 02:59:59 didn't 'exist' when moving from CST to CDT. calc.zeng uses as.POSIXct() with your system's timezone to ensure that it is in POSIX format, which it doesn't know what to do with 2017-03-12 02:00:00 in American/Chicago timezone since that didn't exist.

I think an improvement would be to remove as.POSIXct() from calc.zeng and replace it with an error that says that datetime must be in POSIXct format before running code. Similar to addNAs function: https://github.com/GLEON/LakeMetabolizer/blob/d0a843654bdac71f4cc18c7c7a0e0f9907b53408/R/addNAs.R#L14-L24

jzwart commented 6 years ago

what do others think? @lawinslow @rBatt @riwoolway

rBatt commented 6 years ago

You're probably right, Jake. Dealing with a time change probably requires having the user do some sort of standardization on their dates ahead of time, or implementing a localized time conversion. The second one would be a nightmare. Should probably make sure the units fall in line with the sun rise/ set function, which I believe uses latitude and time of year to determine time of day for the rise/set. What units are those times in? Gosh, I haven't thought about that in a while, because I didn't usually have to deal with a time change ...

On Tue, Jun 5, 2018 at 5:14 PM, Jake Zwart notifications@github.com wrote:

what do others think? @lawinslow https://github.com/lawinslow @rBatt https://github.com/rBatt @riwoolway https://github.com/riwoolway

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GLEON/LakeMetabolizer/issues/130#issuecomment-394862574, or mute the thread https://github.com/notifications/unsubscribe-auth/AFXrf0g2zuia7Up6dlyEin77dXrwiIxrks5t5vSqgaJpZM4UXy4y .

-- Ryan D. Batt, Ph.D. Aquatic Ecologist Postdoctoral Fellow, National Academy of Sciences, US EPA Website: batt.limnology.wisc.edu Pronouns: he/ him

boegmanl commented 6 years ago

Why not just put everything in GMT? Thats what Environment Canada does for moorings to avoid daylight savings issues.

Leon Boegman Associate Professor & Graduate Coordinator Environmental Fluid Dynamics Laboratory Department of Civil Engineering Queen's University at Kingston www.civil.queensu.ca/Research/Hydrotechnical/Leon-Boegman/http://www.civil.queensu.ca/Research/Hydrotechnical/Leon-Boegman/

Associate Editor: Limnology & Oceanography Associate Editor: Journal of Great Lakes Research

On Jun 5, 2018, at 5:14 PM, Jake Zwart notifications@github.com<mailto:notifications@github.com> wrote:

what do others think? @lawinslowhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flawinslow&data=02%7C01%7Cleon.boegman%40queensu.ca%7Cd8e2b278b1084950ea9f08d5cb294d78%7Cd61ecb3b38b142d582c4efb2838b925c%7C1%7C0%7C636638300622441884&sdata=D7kCd7n2UaswmhI2JtinYuL0Fw5w7g6r%2BlM0gzp4LtE%3D&reserved=0 @rBatthttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FrBatt&data=02%7C01%7Cleon.boegman%40queensu.ca%7Cd8e2b278b1084950ea9f08d5cb294d78%7Cd61ecb3b38b142d582c4efb2838b925c%7C1%7C0%7C636638300622441884&sdata=qT7frp2mJMgEAv7MgidGHEO1Lw7cXTl1uXEw653tvf8%3D&reserved=0 @riwoolwayhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Friwoolway&data=02%7C01%7Cleon.boegman%40queensu.ca%7Cd8e2b278b1084950ea9f08d5cb294d78%7Cd61ecb3b38b142d582c4efb2838b925c%7C1%7C0%7C636638300622441884&sdata=BmUHOlIoAVvvIsgj6OndDmv0XUFn9MmGThUMcHysxfQ%3D&reserved=0

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FGLEON%2FLakeMetabolizer%2Fissues%2F130%23issuecomment-394862574&data=02%7C01%7Cleon.boegman%40queensu.ca%7Cd8e2b278b1084950ea9f08d5cb294d78%7Cd61ecb3b38b142d582c4efb2838b925c%7C1%7C0%7C636638300622441884&sdata=F9Y4UiLkjmIf8Qh0hr3Bj1PI9d94x7gi9lwifHWxxV4%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FALSkvzU_x_zMojxu4f9uEe6FaaaGa2JSks5t5vSrgaJpZM4UXy4y&data=02%7C01%7Cleon.boegman%40queensu.ca%7Cd8e2b278b1084950ea9f08d5cb294d78%7Cd61ecb3b38b142d582c4efb2838b925c%7C1%7C0%7C636638300622441884&sdata=HF46D9TOtkVSD%2FkKFf%2FypquDAea5mY0xabkaMEEfB9I%3D&reserved=0.

jzwart commented 6 years ago

If the user wants to put everything into GMT then that is an option for avoiding daylight savings issues, but I don't think the functions should have a timezone set to GMT. I think the user should get out the time zone that they put into the function.

rBatt commented 6 years ago

The trick is getting things from some unknown or arbitrary time zone into something like GMT. Once in GMT, everything is easy, and I agree it's a simpler standard to work with. But I also agree that maybe we don't want to take on the task of converting everything to GMT ourselves.

Quick Aside: I worry about what time zone our sun.rise.set uses, though looking through the code again I think it's all standardized to local hours of day. So maybe not an issue.

Interestingly, in R's as.POSIXct, if you just specify the time zone code for the location, it'll automatically specify whether it's standard time or daylight time.

For example:

as.POSIXct("02/09/2018", format="%m/%d/%Y", tz="America/New_York")

"2018-02-09 EST"

as.POSIXct("06/09/2018", format="%m/%d/%Y", tz="America/New_York")

"2018-06-09 EDT"

Once a user has their data in POSIX format with the time zone specified, they might have an easier time solving the problem.

Maybe the solution here is to leave our code untouched, but start compiling a list of examples or FAQ's in the GitHub wiki that we can reference to show people how to make such a conversion in their own code when using LakeMetabolizer. I.e., just give an example with this initial problem, and show how to solve it.