Skip to content

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented Apr 16, 2020

#677

Currently we have implemented the methods and constructors needed to access parameters and variables in the top level composite components of a model with a call like

m[:top, :par_name

next steps are to add more comprehensive testing and think about corner cases like renaming. also, do we want to be able to use this syntax for lower levels i.e. if :B is a child composite component of :top but not in the comps_dict of m.mi, should we be able to do

m[:B, :par name]

@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #704 into master will increase coverage by 0.28%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   79.74%   80.02%   +0.28%     
==========================================
  Files          38       38              
  Lines        2839     2854      +15     
==========================================
+ Hits         2264     2284      +20     
+ Misses        575      570       -5     
Flag Coverage Δ
#unittests 80.02% <96.29%> (+0.28%) ⬆️
Impacted Files Coverage Δ
src/core/instances.jl 79.13% <90.00%> (+0.82%) ⬆️
src/core/build.jl 100.00% <100.00%> (+4.10%) ⬆️
src/core/types/instances.jl 71.18% <100.00%> (-0.95%) ⬇️

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 2b70c8f...8719ac0. Read the comment docs.

@lrennels lrennels marked this pull request as ready for review April 17, 2020 04:51
@lrennels lrennels requested a review from corakingdon April 17, 2020 04:51
Copy link
Collaborator

@corakingdon corakingdon left a comment

Choose a reason for hiding this comment

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

Could you also update the "docs/src/internals/strucutres_3_instances.md" file just to reflect the two new fields for CompositeComponentInstances?

@lrennels lrennels requested a review from corakingdon April 17, 2020 22:35
Copy link
Collaborator

@corakingdon corakingdon left a comment

Choose a reason for hiding this comment

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

Looks great! The one thing I'm wavering on now is whether parameters and variables are the best names for the new fields in the ComposteComponentInstance. Maybe it should be something like parameter_refs instead. But maybe we should just merge this now and then open a new issue to ask David's thoughts on that later?

@lrennels
Copy link
Collaborator Author

lrennels commented May 4, 2020

Hmm so would the idea of not naming them variable and parameter be that they are difference since they are immutable at this level and just references to lower components as opposed to unique ones?

@corakingdon
Copy link
Collaborator

yeah mostly the second point-- just that they don't actually store parameter/variable values, but references to them. minor point, but I feel like David would have a clear opinion on it so we could just revisit it later!

@lrennels lrennels merged commit 7c6c43b into master May 4, 2020
@lrennels lrennels deleted the datum_access branch May 4, 2020 20:59
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