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

Updates to Tech Note Photosynthesis chapter #1440

Merged
merged 3 commits into from
Jul 28, 2021
Merged

Conversation

olyson
Copy link
Contributor

@olyson olyson commented Jul 26, 2021

Description of changes

Updates to photosynthesis chapter in technical note per discussion in #1398.

Specific notes

Correct some errors and clarify derivation of stomatal conductance quadratic equation for Medlyn method.
Contributors other than yourself, if any: @djk2120 @heraldn

CTSM Issues Fixed (include github issue #): Documentation aspects of #1398

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any:
I did a local build on the documentation.

@olyson olyson added the documentation additions or edits to user-facing documentation label Jul 26, 2021
@ekluzek
Copy link
Collaborator

ekluzek commented Jul 26, 2021

@billsacks this seems like another candidate to just merge to master without testing. Is there any reason not to do that? The next tag should include it in their tag documentation.

@olyson if I understand the process we do need to do a build and push the results to the PR for the changes to automatically show up in the Tech note. Is that something that you could do?

@olyson
Copy link
Contributor Author

olyson commented Jul 26, 2021

Yes, I can push the build to gh-pages once this is approved.

@billsacks
Copy link
Member

Yup, fine to merge directly to master. @olyson does it need review from anyone first?

@olyson
Copy link
Contributor Author

olyson commented Jul 26, 2021

I'm not sure how to make the changes visible to someone who wants to review it since it is a local build on my windows machine. Not sure also who would have time, perhaps @danicalombardozzi or @djk2120 .

@billsacks
Copy link
Member

@olyson I was imagining someone could review the changes to the documentation source. I also feel it would be okay to push the new build to the official site (for the master version of the docs) for the sake of review. But mainly I was wanting your input on whether you wanted a review or not – if you don't feel one is needed, then neither do I.

@olyson
Copy link
Contributor Author

olyson commented Jul 26, 2021

Ok, I think what I will do is to push the new build to the official site and ask Nick Herold to review it since he pointed out some of these documentation errors. I'll ask him to review it as part of #1398. He doesn't seem to show up as a potential reviewer on pull requests.

@billsacks
Copy link
Member

Sounds good. Only people that are on one of the CTSM GitHub teams can be added as PR reviewers. I'm always happy to add others if you feel others should be added. (The lowest permission level we give people is "Triage", which only gives a few permissions beyond what the public at large can do: see https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-permission-levels-for-an-organization).

@billsacks
Copy link
Member

billsacks commented Jul 26, 2021

But I'll also add: in situations where you want a one-time review from someone who isn't a typical contributor / reviewer of CTSM, I would also just mention them in a comment and ask for an unofficial review that way. So what you have done here is fine.

@olyson
Copy link
Contributor Author

olyson commented Jul 28, 2021

Ok, I got a review from Nick and made a couple of corrections. I think this is good to go now.

@billsacks billsacks merged commit 84e970f into ESCOMP:master Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation additions or edits to user-facing documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants