Skip to content

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented May 6, 2021

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #829 (5881f4d) into master (e0d3c71) will increase coverage by 0.14%.
The diff coverage is 89.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #829      +/-   ##
==========================================
+ Coverage   84.11%   84.25%   +0.14%     
==========================================
  Files          40       40              
  Lines        3551     3780     +229     
==========================================
+ Hits         2987     3185     +198     
- Misses        564      595      +31     
Flag Coverage Δ
unittests 84.25% <89.57%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Mimi.jl 100.00% <ø> (ø)
src/components/adder.jl 0.00% <ø> (ø)
src/core/defcomp.jl 80.11% <ø> (-2.38%) ⬇️
src/core/defcomposite.jl 90.90% <ø> (ø)
src/core/instances.jl 80.53% <ø> (ø)
src/core/references.jl 73.52% <ø> (ø)
src/core/time.jl 93.51% <ø> (ø)
src/core/types/core.jl 92.85% <ø> (ø)
src/core/types/model.jl 86.66% <ø> (ø)
src/explorer/explore.jl 60.29% <ø> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0d3c71...5881f4d. Read the comment docs.

@lrennels
Copy link
Collaborator Author

lrennels commented May 12, 2021

Per discussion 5/12/2021:

  • replace! behavior

How do we want replace! to handle new defaults and new parameters?

When we replace a component with a new component, but choose to maintain all old connections with the reconnect = true keyword arg (defaults to true) we will maintain the old default settings, and not replace them with the new component's defaults. That is the existing behavior, tests already there.

That said, we might need to add in some new nothing unshared external parameters to our external parameter list, so upon the call of add_comp! we should add those but not add any for parameters that are already connected. This means adding a check in _initilialize_parameters! to see if an external parameter and connection already exists.

  • composite defaults behavior

How do we want to handle defaults from composites?

Each parameter in the tree of a composite component will get an unshared external parameter, including defaults, so the behavior should not be different than non-composite components. This is already the case, just adding testing.

We do need to think through parameters bubbled up using the collision-avoiding syntax like superp1 = Parameter(A.p1, B.p1), and whether in that case superp1 is a shared parameter or an unshared, unnamed parameter. As of now it is the latter.

  • Monte Carlo

Question: How do we (1) not break old behavior where we gave a parameter distribution to a default value (and thus shared named external parameter) in the simulation definition and (2) handle unshared parameters and defaults goign forward.
Answer: We will add a special check for the p = Normal(0,1) syntax such that if we can't find p we'll go look to see if there is a unique component parameter with a default that we can use that for ... if not we can error and suggest the new syntax. Our new syntax will be comp.parameter = Normal(0.,1) for non shared parameters. Sometimes we cannot resolve this conflict, so we have to update the model ie. MimiPAGE2009 and MimiPAGE2020

@lrennels
Copy link
Collaborator Author

lrennels commented May 28, 2021

Next Steps ...

  • external API additions and a new testing file for these updates test_new_param_API.jl
  • update_param!(m, comp, param, value and its @delegate methods
  • add_shared_parameter and its delegate methods
  • update_params! for unshared parameters and its delegate methods
  • do some work getting rid of "external parameter" replaced with "shared model parameter" or "unshared model parameter"
  • phase out set_param! from example code and tests, replacing it with the new syntax
  • set_leftover_params! for unshared parameters (and maybe rename it to update_leftover_params) and its delegate methods
  • double check docs locally (https://juliadocs.github.io/Documenter.jl/stable/man/guide/ by running make.jl and serving with LiveServe), try to add more docstrings that link
  • double check all error and warning messages for usefulness
  • re-test all dependencies and models

@lrennels lrennels marked this pull request as ready for review June 1, 2021 23:06
@davidanthoff davidanthoff merged commit 2c9d71e into master Jun 14, 2021
@davidanthoff davidanthoff deleted the params branch June 14, 2021 19:03
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