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

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

Merged
merged 4 commits into from
Oct 9, 2020
Merged

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

merged 4 commits into from
Oct 9, 2020

Conversation

clnsmth
Copy link
Contributor

@clnsmth clnsmth commented Sep 15, 2020

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.

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 jzwart self-requested a review September 15, 2020 20:34
@jzwart
Copy link
Member

jzwart commented Sep 29, 2020

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
Copy link
Contributor Author

clnsmth commented Oct 5, 2020

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

@jzwart
Copy link
Member

jzwart commented Oct 8, 2020

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

Updating demo k_600 figure to work with new data.frame output from sun.rise.set() function
@clnsmth
Copy link
Contributor Author

clnsmth commented Oct 8, 2020

@jzwart, did that work?

@jzwart
Copy link
Member

jzwart commented Oct 9, 2020

Yup looks good, thanks!

@jzwart jzwart merged commit 5e42781 into GLEON:master Oct 9, 2020
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