Repository navigation
[1.9.x] Drop trailing commas that made twelve argsDict values 1-tuples - #45
Merged
Merged
Conversation
`argsDict["k"] = arr,` stores (arr,). The xt::pyarray caster converts the
tuple instead of rejecting it (PyArray_FromAny with FORCECAST), and
pybind11 tries the int overload of ArgumentsDict.__setitem__ first. So:
- An int array became a (1, ...) copy with the same flat layout. Kernels
read it through .data(), so the results were right, off a needless copy:
PresInc.getResidual elementBoundaryLocalElementBoundariesArray
RANS2P_IB.getJacobian sdInfo_u_w_rowptr
RANS3PF.getJacobian sdInfo_u_w_rowptr, sdInfo_w_v_rowptr
RANS3PSed.getJacobian sdInfo_u_v_rowptr
Richards.getResidual/getJacobian elementMaterialTypes
m_comp_co2.getResidual/getJacobian elementMaterialTypes
All of these run unconditionally. test_clsvof_with_rans3pf (PresInc,
RANS3PF) and griffiths_lane_6 (Richards) reach theirs and pass because
of the conversion. RANS3PSed, RANS2P_IB and m_comp_co2 have no test.
- A double array was truncated into the *int* dict, so the kernel's
args.array<double>("elementDiameter") threw "key elementDiameter not
found". That happens on every call of RANS2P_IB.getJacobian,
RANS2P_IB.beamStep and SW2D.getJacobian. No test runs either model.
On main, where proteus::pyarray rejects tuples, these were fixed by
27c00b0 and the companion main PR. This applies the same comma removals.
The Richards delta_x_ij reordering in 27c00b0 is not needed here, because
1.9.x already allocates it before building argsDict.
richards/ADR.py has three more. They are left for erdc#1285 (its
extension is not built). The new test lists ADR.py as a known exception and
fails once it is fixed, so the exception gets removed.
test/test_argsdict_values.py walks the installed proteus sources with ast
and rejects any tuple, list, set, dict, comprehension, None or string
assigned into an ArgumentsDict.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 12, 2026
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.
argsDict["k"] = arr,stores the 1-tuple(arr,). On 1.9.x this does not raiseTypeError: thext::pyarraycaster converts the tuple instead (PyArray_FromAnywithFORCECAST), and pybind11 tries theintoverload ofArgumentsDict.__setitem__first. I checked this against an installed 1.9.x build (proteus-arm64-verify):a["k"] = (np.zeros((4,2),'i'),)is stored as shape(1,4,2), anda["d"] = (np.array([0.5,1.7]),)lands iniarrayas[[0,1]], sodarray["d"]raisesRuntimeError: key d not found.Int arrays: silent copy, correct results. The kernels read through
.data(), so they saw the right numbers.mprans/PresInc.py:861getResidualelementBoundaryLocalElementBoundariesArraytest_clsvof_with_rans3pfmprans/RANS3PF.py:2729,:2741getJacobiansdInfo_u_w_rowptr,sdInfo_w_v_rowptrtest_clsvof_with_rans3pfrichards/Richards.py:1599,:1962getResidual,getJacobianelementMaterialTypesmprans/RANS3PSed.py:1998getJacobiansdInfo_u_v_rowptrmprans/RANS2P_IB.py:1397getJacobiansdInfo_u_w_rowptrm_comp_co2/m_comp_co2.py:3830,:4677getResidual,getJacobianelementMaterialTypesDouble arrays: hard failure. The value is truncated into the int dict, so the kernel's
args.array<double>("elementDiameter")throws "key not found" on every call.mprans/RANS2P_IB.py:1343getJacobianmprans/RANS2P_IB.py:1574beamStepmprans/SW2D.py:825getJacobianAll twelve sites run unconditionally; none depends on
nSpaceor a coefficient option. Tests "missed" them for two reasons: the conversion hides the int cases, and nothing runs RANS2P_IB, SW2D, RANS3PSed or m_comp_co2.On main, these were fixed by 27c00b0 and #44. Only the comma removals are applied here. The
delta_x_ijreordering in 27c00b0 isn't needed, because 1.9.x'sRichards.getResidualalready allocates it before buildingargsDict.richards/ADR.py(:1160/:1341/:1587) is intentionally left alone for erdc#1285. The new test lists it inKNOWNand fails once it is fixed, which forces the exception to be removed.Guard
test/test_argsdict_values.pyis the same AST scan as #44, plus theKNOWNexemption. Both tests pass on this branch; the scan fails on unfixedorigin/1.9.x(12 sites).PresInc.py also appears in open #43, but in a different hunk.
🤖 Generated with Claude Code