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

Consolidate text between UG and args spec (part 1 of 3) #42

Merged
merged 25 commits into from
Nov 20, 2021

Conversation

emlys
Copy link
Member

@emlys emlys commented Aug 19, 2021

This includes:

  • Carbon
  • Coastal Blue Carbon
  • Coastal Blue Carbon Preprocessor
  • Coastal Vulnerability
  • Crop Production Percentile
  • Crop Production Regression
  • DelineateIt

@emlys emlys marked this pull request as ready for review August 19, 2021 18:42
@emlys emlys force-pushed the task/31/models-A-D branch 3 times, most recently from a908821 to 73f2b4e Compare August 20, 2021 19:10
@emlys emlys self-assigned this Aug 25, 2021
Copy link
Contributor

@davemfish davemfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up adding a couple comments here as I looked at some the rendered chapters. I'm just going to submit now so you can see those comments @emlys . I've not really "reviewed" this PR though. I'd rather wait until after #34 since all those changes are in here also. Hope that makes sense!

source/coastal_vulnerability.rst Outdated Show resolved Hide resolved
source/coastal_vulnerability.rst Show resolved Hide resolved
Comment on lines 13 to 14
Units
~~~~~
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to make this less large of a section? When viewing it, I first thought it was it's own top level header and wondered why it was capitalized and not underlined. Then I realized it was part of the number category. Could just be me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see answer in #34

Comment on lines 79 to 80
Bands
~~~~~
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing as above, could it be clearer this falls under the raster category?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #34

source/input_types.rst Outdated Show resolved Hide resolved
source/conf.py Outdated Show resolved Hide resolved
Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @emlys , this looks awesome. This is a huge improvement and I love seeing the integration come together.

@emlys emlys requested a review from davemfish August 31, 2021 00:05
@emlys emlys changed the base branch from release/3.10 to main September 22, 2021 16:50
@emlys emlys changed the base branch from main to release/3.10 September 22, 2021 16:51
Copy link
Contributor

@davemfish davemfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These chapters look great! Very exciting!

Comment on lines 47 to 49
.. math:: value\_seq_x=V\frac{sequest_x}{yr\_fut-yr\_cur}\sum^{yr\_fut-yr\_cur-1}_{t=0}\frac{1}{\left(1+\frac{r}{100}\right)^t\left(1+\frac{c}{100}\right)^t}
:label: carbon_value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the variables in this equation be defined afterwards? Like: where V is this and that ,c is that and this, etc...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree, it'd be nice to include these values here. It does look like these values are defined in the parameter descriptions below ... a little obfuscated, but at least they are defined somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I added variable definitions. Though, I didn't change this equation, just moved it from a different section of the text. There are many other places in the UG where variables aren't defined, or are re-used to mean different things, and fixing all that will be a bigger project.

source/carbonstorage.rst Outdated Show resolved Hide resolved
source/delineateit.rst Outdated Show resolved Hide resolved
Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @emlys , I just had some trivial suggestions. I'll approve ahead of time because they're so trivial. Looks great!

Copy link
Member

@phargogh phargogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh this really slipped through the cracks. I'm so sorry about that! I just had two comments and the ?? is the main thing for me.

Comment on lines 47 to 49
.. math:: value\_seq_x=V\frac{sequest_x}{yr\_fut-yr\_cur}\sum^{yr\_fut-yr\_cur-1}_{t=0}\frac{1}{\left(1+\frac{r}{100}\right)^t\left(1+\frac{c}{100}\right)^t}
:label: carbon_value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree, it'd be nice to include these values here. It does look like these values are defined in the parameter descriptions below ... a little obfuscated, but at least they are defined somewhere.

source/coastal_blue_carbon.rst Outdated Show resolved Hide resolved
@emlys emlys requested a review from phargogh November 17, 2021 00:06
Copy link
Member

@phargogh phargogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK great!

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.

4 participants