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

Two stream gray docs #176

Merged
merged 26 commits into from
Sep 23, 2020
Merged

Two stream gray docs #176

merged 26 commits into from
Sep 23, 2020

Conversation

ntlewis
Copy link
Contributor

@ntlewis ntlewis commented Jul 16, 2020

This PR addresses issue #132 by adding documentation for two_stream_gray_rad.

@ntlewis ntlewis linked an issue Jul 16, 2020 that may be closed by this pull request
@RuthG
Copy link

RuthG commented Jul 20, 2020

Hi Neil,

I've just taken a quick look over this, the structure and description of what each scheme does look good to me! Just a few typos to flag up:
l35: available
l37: available
l202: diurnal
l402: calculations

@ntlewis
Copy link
Contributor Author

ntlewis commented Jul 20, 2020 via email

@RuthG
Copy link

RuthG commented Jul 20, 2020 via email

@matthewjhenry
Copy link
Contributor

Great job @ntlewis and @RuthG. This is very thorough and helpful documentation! :)

Happy to push when these comments are considered (some are mere suggestions).

Comments:

  • typos 'available'
  • L. 67: 'relative to the :math:^{k} term' comes out weird when it's rendered, as the k exponent is just floating about. Change to 'first' and 'second' term or something else?
  • L. 72: typo default
  • L. 150: spectral and \ in front of mu (doesn't render correctly)
  • L. 202: available
  • L. 207: distribution, incoming
  • L.220: received, diurnal
  • L. 256: same as line 67
  • L. 291: maybe 'Sets whether insolation is diurnally-and-seasonally averaged (TRUE), or time dependent (FALSE). Default FALSE.'
  • L. 296: determining
  • L. 301: 'would achieved' to 'would set to'?
  • L. 323: They
  • L. 329: calculate
  • L. 402: calculations

@ntlewis
Copy link
Contributor Author

ntlewis commented Sep 23, 2020

Hi @matthewjhenry and @RuthG, I've corrected each of the typos you spotted.

Ruth: I haven't yet done anything regarding the hard coding of the BOG CO2 coeff, or the possibility to include Ozone in the Geen scheme. As neither of these changes have been made to the code yet, I've left any changes relating to them out of the docs. I have, however, included a note saying that the Geen scheme is only stable for fixed SSTs.

I think this is ready to go now, once the Travis check passes and you guys approve it. Would be good to get it in soon :)

Neil

@RuthG
Copy link

RuthG commented Sep 23, 2020

Could you change the GEEN expected by to 30/11/2020 please! There's not much work needed but I've got a lot of commitments the next couple of months and would rather not set myself up to fail immediately here!

  • just saw my refs have been moved not deleted so I've removed my previous comment, my bad. Just change the occurrences of the date and I'll merge :)

@ntlewis
Copy link
Contributor Author

ntlewis commented Sep 23, 2020

@RuthG occurrences of date changed. Feel free to merge once the Travis build has passed again.

@ntlewis ntlewis merged commit fab6031 into ExeClim:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs priority:high High-priority task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

two_stream_gray_rad_mod
3 participants