Add defsim clarifications to docs #781
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #781 +/- ##
=======================================
Coverage 78.73% 78.73%
=======================================
Files 35 35
Lines 2996 2996
=======================================
Hits 2359 2359
Misses 637 637
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
corakingdon
reviewed
Nov 10, 2020
docs/src/howto/howto_3.md
Outdated
|
|
||
| ### Apply RVs to model parameters | ||
|
|
||
| **For all applications in this section, it is important to note that for each trial, a random variable on the right hand side of an assignment will take on the value of a *single* draw from the given distribution. This means that even if the random variable is applied to more than one parameter on the right hand side (such as assigning to a slice), each of these parameters will be assigned the same value, not different draws from the distribution.** |
There was a problem hiding this comment.
should this be "is applied to more than one parameter on the left hand side"?
docs/src/howto/howto_3.md
Outdated
Comment on lines
79
to
82
| - `param += RV` replaces the values in the parameter with the sum of the original value and the value of the RV for the current trial. | ||
| - `param *= RV` replaces the values in the parameter with the product of the original value and the value of the RV for the current trial. | ||
| - `param += RV` replaces the values in the parameter with the sum of the previous trial's value and the value of the RV for the current trial. | ||
| - `param *= RV` replaces the values in the parameter with the product of the previous trial's value and the value of the RV for the current trial. |
There was a problem hiding this comment.
I don't think this is correct? I think it is the product of the original value and the random variable? not sure though. But that would be mean it shouldn't be thought of as a random walk afterall
docs/src/tutorials/tutorial_3.md
Outdated
| ``` | ||
|
|
||
| Next, create a dictionary `params` with one entry `(k, v)` per external parameter by name `k` to value `v`. Each key `k` must be a symbol or convert to a symbol matching the name of an external parameter that already exists in the model definition. Part of this dictionary may look like: | ||
| Now that you have changed the time dimension, you have a mismatch between the time labels attached to your parameters and the time lables used by the model. Thus, **you must update at least all parameters with a `:time`** dimension and use the explicit `update_timesteps=true` flag to get the time labels on the parameters to match those in the model. This is required even in cases where you do not want to change the parameter values themselves (see the forum question [here](https://forum.mimiframework.org/t/update-time-index/134/5)) for an in-depth explanation of this case. You may of course also update parameters without a `:time` dimension as desired. |
docs/src/tutorials/tutorial_5.md
Outdated
| ```julia | ||
| param1 = Normal(0, 0.8) | ||
| ``` | ||
| **It is important to note** that for each trial, a random variable on the right hand side of an assignment, be it using an explicitly defined random variable with `rv(rv1)` syntax or using shortcut syntax as above, will take on the value of a **single** draw from the given distribution. This means that even if the random variable is applied to more than one parameter on the right hand side (such as assigning to a slice), each of these parameters will be assigned the same value, not different draws from the distribution |
There was a problem hiding this comment.
same "left hand side" correction I think
docs/src/tutorials/tutorial_5.md
Outdated
| share = Uniform(0.2, 0.8) | ||
| # you can use the *= operator to replace the values in the parameter with the | ||
| # product of the previous trial's value and the value of the RV for the current |
There was a problem hiding this comment.
TBD if it's previous trial or original value
corakingdon
approved these changes
Nov 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From the forum: https://forum.mimiframework.org/t/mcs-assigning-random-variables-to-indexed-variable/129/4
You point to the text "param *= RV replaces the values in the parameter with the product of the original value and the value of the RV for the current trial " from the How-To-Guide 3. Since, as you agree, it operates as a random walk, it seems that the link should not say “product of the original value”, but rather “product of the value in the previous trial”? This, of course, leaves the question of whether there is a way to have it actually use the original value, which I think is more natural, i.e. to select a value in each trial that fall between 0.9 and 1.1 times the default value.
By the way, I patterned the line “s[2020:5:2050] *= Uniform(0.9, 1.1)” after the line “sigma[2020:5:2050, (Region2, Region3)] *= Uniform(0.8, 1.2)” in tutorial 5. For each trial, I believe this assigns the same value for sigma for all the years 2020, 2025, . . . , 2050 in Regions 2 and 3. It might be good to make this clear. Some people, e.g., me might accidentally interpret this line as implying that it chooses a separate value for each year and each region.
@daler6 is is a fairly lightweight adjustment to the documentation as you suggested, I will consider a larger overhaul but wanted to start with this since (1) is especially misleading
@ckingdon95 can you look at this and then confirm that this in fact what's being employed (I'm fixing the broadcasting error in another PR)