OCM: improve simulation context setup #2279
Conversation
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
| # KDM 5/20/19: crudely using default here because it is a stateless parameter | ||
| # and there is a bug in setting parameter values on init, see TODO note above | ||
| # call to self._instantiate_defaults around component.py:1115 | ||
| if self.defaults.search_statefulness: | ||
| new_context = self._set_up_simulation(context, control_allocation) | ||
| new_context = self._set_up_simulation(context, control_allocation, alt_controller) | ||
| else: | ||
| new_context = context |
There was a problem hiding this comment.
wouldn't this pollute the 'value' parameters of components
There was a problem hiding this comment.
Sorry, I'm not seeing what part you're referencing here. Is it a new change or something in general to simulation contexts?
(I do see the unintentional loss of check_simulation_storage=True arg to delete_contexts)
There was a problem hiding this comment.
it's not related to the change. the else branch just reuses the base context.
wouldn't the 'value' parameter now include results from simulations stored under the base context?
There was a problem hiding this comment.
Yes, it's the intended behavior if search_statefulness is set to False. I think @davidt0x had a use case for this
| if self.agent_rep.controller is None: | ||
| try: | ||
| alt_controller = context.composition.controller | ||
| except AttributeError: |
There was a problem hiding this comment.
is this supposed to catch both missing context.composition and context.composition.controller?
if it's only the latter.
alt_controller = getattr(context.composition, 'controller', None)
would be more readable.
There was a problem hiding this comment.
It was meant to catch both. I'm unsure if there's a case where this code would be run in a context with no composition set, but if that happened I think it should handle it.
There was a problem hiding this comment.
is the absence of composition in context legal in this situation? or is it punting the error to be reported later?
There was a problem hiding this comment.
I think in that case, it's ok and intended not to deal with it. alt_controller would just be one other possibility to try in the case that agent_rep has no controller. If it's not ok for context to have no composition, then I think that should be reported elsewhere.
kmantel
force-pushed
the
devel
branch
2 times, most recently
from
32d0deb
to
1d07e8c
Compare
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
1 similar comment
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
- initialize context for when an agent_rep uses a controller other than its self.controller (ex. test_nested_composition_as_agent_rep) - add Composition._initialize_as_agent_rep and Composition._clean_up_as_agent_rep
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
initialize context for when an agent_rep uses a controller other than
its self.controller
(ex. test_nested_composition_as_agent_rep)
add Composition._initialize_as_agent_rep and
Composition._clean_up_as_agent_rep