Register implicit residual metadata and disambiguate partial shape lookups - #83
Merged
Merged
Conversation
…okups Discipline.add_output appends a second VariableMetaData typed kResidual for implicit disciplines, but never registered that key in _declared, so the duplicate-name index covered the output and not its residual. The residual key is now registered alongside the output, and the dead kOutput assignment that preceded it is gone. Both preallocate_partials (server) and _recover_partials (client) keyed the shape lookup on the variable name alone. The residual comes after the output in the metadata list, so it shadowed the output and every partial of an implicit discipline was sized against the residual entry regardless of which side of the pair was being resolved. Both sites now build a (type, name) index and resolve the function against the residual (falling back to the output) and the variable against the input (falling back to the output), which is how SetVariableShapes already indexes. An unknown name now raises PhiloteValidationError rather than KeyError. Closes #79.
…-metadata-index # Conflicts: # CHANGELOG.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #79.
Problem
Discipline.add_outputappends a secondVariableMetaDatatypedkResidualfor implicit disciplines, but never registered that key in_declared. The duplicate-name index added in #74 therefore covered the output and not its residual, so the index and the metadata list could drift.The same name collision showed up downstream:
DisciplineServer.preallocate_partialsandDisciplineClient._recover_partialsboth builtshapes = {var.name: tuple(var.shape) for var in ..._var_meta}. The residual comes after the output in the list, so it overwrote the output entry and every partial of an implicit discipline was sized against the residual, on both sides of the pair. Benign while the two shapes agree, wrong the moment they diverge.Changes
add_outputregisters(kResidual, name)alongside(kOutput, name), and the deadres_meta.type = kOutputassignment that was immediately overwritten is gone.philote_mdo/utils/helper.py:build_shape_index(keyed on(type, name)),get_function_shape(residual first, then output) andget_variable_shape(input first, then output). A partial is always d(function)/d(variable), where the function is an output for an explicit discipline or a residual for an implicit one, and the variable is an input or, for implicit disciplines, an output — the lookup order encodes exactly that.SetVariableShapes, which already indexes by(type, name).PhiloteValidationErrorinstead of a bareKeyError.The wire protocol is unchanged; this is all local metadata bookkeeping.
Tests
Six new tests: residual registration and redeclaration after
_clear_data(test_discipline.py), and the diverged-shape lookup plus the unknown-name error on both the server and the client. All six fail against the pre-fix source and pass after. Full suite: 315 passed.Changelog
Two entries under
[Unreleased] → Bug Fixes.