search

GLEON / LakeMetabolizer

Collection of Lake Metabolism Functions
16 stars 10 forks source link

Change sun.rise.set() output from matrix to data.frame #139

Closed clnsmth closed 4 years ago

clnsmth commented 4 years ago

This fix maintains the index order of returned sunrise/sunset values and the associated POSIX classes while adding the column names ‘sunrise’ and ‘sunset’. A search of the code base reveals this function is used by is.day() and add.night(), which index values according to order, therefore this fix does not break any linked processes within LakeMetabolizer.

Additionally, the calculation methods imply returned values in standard time. The function metadata now explicitly states this.

jzwart commented 4 years ago

Hi @clnsmth , thank you for submitting this PR and detailed notes in issue #127. Sorry for the delay in reviewing this. All looks good! I rebuilt locally and reproduced the output in the demo figures, which matched the results detailed in the Winslow et al 2016 manuscript. Only issue is a conflict of the add_night function for the k_600 figure, which throws an error with the new data.frame output. I'm adding a PR to your PR for suggested changes. And thank you for adding the tests to this package, that's good practice that we should expand upon for LakeMetabolizer.

Merging this PR should close #127, but @clnsmth brings up a good issue with sunrise and sunset calculations in LakeMetabolizer - see #138 for more details.

clnsmth commented 4 years ago

OK @jzwart. Let me know if there's anything I can help with.

jzwart commented 4 years ago

@clnsmth, I think if you merge my PR into yours, then I can merge this one.

clnsmth commented 4 years ago

@jzwart, did that work?

jzwart commented 4 years ago

Yup looks good, thanks!