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

pheTransitionDates function initial commit #4

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

kjones13
Copy link

@sokole first draft of the new phenology function, ready for initial reveiw

@sokole
Copy link
Collaborator

sokole commented Sep 20, 2024

@kjones13

  • The convention we are using for function names is verbNoun. Like stackPlantPresence. Maybe calcPheTransitionDates?
  • The convention we've adopted for this package is for functions to take list objects from neonUtilities as inputs. For example, in Dave's function, the argument is divDataList, because it gets sent the list object for plant diversity that is returned by neonUtilities. Here, maybe we could make the argument pheDataList and the function is expect the phe list object returned by neonUtilities. Then, rather than sending it pheDat$phe_statusintensity, you could just do pheDataList = pheDat in your example.
  • You need to put the package prefix in front of functions used in your package that are not from base R. For example dplyr::arrange() instead of just arrange. If you run devtools::checks(), it will tell you all the instances of functions where we need to do this.

@sokole sokole self-assigned this Sep 20, 2024
@kjones13
Copy link
Author

kjones13 commented Oct 8, 2024

@sokole in the last two commits I addressed all points above, changed name, added namespace, added functionality for using either data list or single table. Ready for re-review?

@sokole
Copy link
Collaborator

sokole commented Oct 9, 2024

Sounds great. I'll take a look. Thanks!

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