docs/fix: follow-ups to #341 Copilot review (remesh swallow contract, evaluate Returns)#350
Open
lmoresi wants to merge 2 commits into
Open
docs/fix: follow-ups to #341 Copilot review (remesh swallow contract, evaluate Returns)#350lmoresi wants to merge 2 commits into
lmoresi wants to merge 2 commits into
Conversation
… review) The broad except Exception is pre-existing behaviour, unified verbatim from three identical try/except sites by the readability wave. The comment claimed the swallow only covers unallocated/size-0 storage; in fact the breadth also hides genuine write failures (shape/broadcast mismatches). Keep the behaviour (wave contract), state the breadth honestly, and add a TODO(DESIGN) to narrow once the unallocated-path exception types are established. Underworld development team with AI support from Claude Code
…opilot review) The WE-04 Returns section said scaling-active evaluation returns a plain ndarray. The implementation (gateway principle) returns unit-aware values whenever the expression carries units, regardless of scaling state; plain ndarray only for unitless expressions. Docstring now matches unwrap_for_evaluate/_evaluate_impl reality. Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
Doc-only follow-ups aligning documentation/comments with established behavior in remesh and unit-aware evaluation: clarifies that _write_var_data()’s broad exception swallow is pre-existing and explicitly documents its wider impact, and corrects uw.function.evaluate()’s return-type documentation to match the unit “gateway principle” implemented in _evaluate_impl.
Changes:
- Update
evaluate()docstring “Returns” section to state that unit-aware outputs are returned whenever the expression carries units, regardless of whether non-dimensional scaling is active. - Expand
_write_var_data()swallow comment to document the intentional skip case vs. the broader exceptions being swallowed, and add a TODO(DESIGN) to narrow/report once exception types are known.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/underworld3/function/functions_unit_system.py | Fix evaluate() return-type documentation to reflect unit-aware wrapping/redimensionalisation behavior in _evaluate_impl. |
| src/underworld3/discretisation/remesh.py | Clarify the existing broad except Exception swallow contract in _write_var_data() and document its implications/TODO. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Doc-only follow-ups to the two GitHub Copilot review comments on #341, which merged before these could land on the PR branch.
remesh.py
_write_var_dataswallow contract (Copilot comment 3533848061): the broadexcept Exceptionis pre-existing behaviour — the helper unified three identical broad try/except sites verbatim, and the snapshot twin had the same breadth before the wave. Behaviour is unchanged (readability-wave contract); the comment now states honestly that the breadth also swallows genuine write failures (shape/broadcast mismatches), not only the intended unallocated/size-0 skip, and carries a TODO(DESIGN) to narrow once the exception types raised by the unallocated-storage.datapaths are established.evaluate()Returns section (Copilot comment 3533848118): the WE-04 docstring said scaling-active evaluation returns a plain ndarray. The implementation follows the gateway principle — unit-aware results (UWQuantity / UnitAwareArray) whenever the expression carries units, regardless of scaling state; plain ndarray only for unitless expressions. Docstring fixed to matchunwrap_for_evaluate/_evaluate_implreality.No behavioural change. File-local tests pass: test_0830_mesh_adapt_variable_transfer, test_0503_evaluate, test_0753_evaluate_nondimensional_scaling (22 passed).
Underworld development team with AI support from Claude Code