Reworked error/warning behavior for set_input_defaults. #1568
Conversation
…t is between a higher level call and a lower level one.
…ry ambiguity errors/warnings at any level in the system tree as long as the ambiguity was ultimately fixed at a higher level.
openmdao/core/tests/test_group.py
Outdated
| @@ -2198,14 +2199,14 @@ def test_conflicting_units(self): | |||
| with self.assertRaises(Exception) as cm: | |||
| p.setup() | |||
|
|
|||
| self.assertEqual(cm.exception.args[0], "Groups 'par.G4' and 'par.G5' added the input 'x' with conflicting 'units'.") | |||
| self.assertEqual(cm.exception.args[0], "Group (<model>): The subsystems G1.G2 and par.G4 called set_input_defaults for promoted input 'x' with conflicting values for 'units'. Call <group>.set_input_defaults('x', units=?), where <group> is the Group named '' to remove the ambiguity.") | |||
There was a problem hiding this comment.
I wonder if we should have special handling for the top level group...
It seems like this message would make more sense to the user if it referred to "the 'model'" instead of "the group named ''", since that's how the user would reference it.
There was a problem hiding this comment.
changed "Group named ''" to "model"
openmdao/core/group.py
Outdated
| @@ -53,6 +53,10 @@ class Group(System): | |||
| Dictionary of input_name: (output_name, src_indices) connections. | |||
| _group_inputs : dict | |||
| Mapping of promoted names to certain metadata (src_indices, units). | |||
| _static_group_inputs : dict | |||
| Group inputs added outside of setup/configure. | |||
| _post_setup_group_inputs : dict | |||
There was a problem hiding this comment.
This name confuses me a little. At first, I thought it meant after the user calls prob.setup. At second, I thought that configure is technically "post setup" too. Maybe pre_configure is better?
| @@ -53,6 +53,10 @@ class Group(System): | |||
| Dictionary of input_name: (output_name, src_indices) connections. | |||
| _group_inputs : dict | |||
| Mapping of promoted names to certain metadata (src_indices, units). | |||
| _static_group_inputs : dict | |||
| Group inputs added outside of setup/configure. | |||
There was a problem hiding this comment.
Where else can we add inputs? Does this just mean after prob.setup?
There was a problem hiding this comment.
Static means everything outside of system setup and configure. If things are added in setup/configure they're considered 'dynamic' and they get blown away if problem setup is called again. Static stuff is never removed.
There was a problem hiding this comment.
This would be a nice comment to put in the code somewhere
More helpful I think that what's there for the definition of static mode:
_static_mode : bool
If true, we are outside of setup.
| group_inputs.extend(ginputs) | ||
| if rank != myrank: | ||
| for p, mlist in ginputs.items(): | ||
| self._group_inputs[p].extend(mlist) |
There was a problem hiding this comment.
The data structure was changed to make error checking easier later.
| origin_prom = submeta['prom'] | ||
| if origin != top_origin: | ||
| simple_warning(f"Group '{top_origin}' did not set a default " | ||
| f"'{key}' for input '{top_prom}', so the value of " |
There was a problem hiding this comment.
is there a test for this warning?
Summary
Ambiguity resolution now happens at the top level only. Ambiguities that are resolved at a higher level are now warnings instead of errors. A warning is issued if a higher level call to set_input_defaults has a missing metadata entry that is supplied from a lower level.
Related Issues
set_input_defaultscalls to a warning. #1565Backwards incompatibilities
None
New Dependencies
None