Repository navigation
fix: keep dash-named parameters in netcdf round trip - #1025
Open
MaykThewessen wants to merge 1 commit into
Open
MaykThewessen wants to merge 1 commit into
MaykThewessen wants to merge 1 commit into
Conversation
…ad_netcdf read_netcdf assigned each data variable and attribute to its container by splitting the key at its last dash. Parameters are stored as parameters-<name>, so a parameter named my-param was assigned to parameters-my and dropped; the same happened to parameter attributes and _multiindex attributes of dimensions with a dash in their name. Keys now belong to the longest known container prefix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 tasks done
Merging this PR will not alter performance
Comparing Footnotes
|
This branch has not been deployed
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.
Note
AI-assisted. Claude wrote the fix and tests, and ran the checks listed below. I reviewed the diff.
Changes proposed in this Pull Request
read_netcdfsilently drops a parameter whose name contains a dash: afterm.parameters["my-param"] = ..., a round trip through netcdf returns the model without it.read_netcdffinds the container of each data variable and attribute by splitting its key at the last dash. That works forvariables-x-var-labels, where the last part is a fixed field name, but parameters are stored asparameters-<name>, soparameters-my-paramends up underparameters-my. Attribute keys have the same problem, which also dropped parameter attributes with a dash and the_multiindexattribute of a dimension with a dash in its name (legacy semantics), so that dimension came back without itsMultiIndex.Each key now belongs to the longest known container prefix, taken from the container names (plus
objectiveandparameters).Overlap with #1021 and with #1024: all three touch
read_netcdfinlinopy/io.pyand merge without textual conflicts. The aux-coords branch derives its own container prefixes by splitting at the last dash, so with both merged as they are, a dash-named parameter yields a bogus prefixparameters-mythat takes the scalar coords ofparametersstarting withmy-. Whichever lands second should drop itsprefixes/scalar_ownerand useowner()from this PR (see details).Checklist
AGENTS.md).doc.doc/release_notes.rstof the upcoming release is included.What was checked (AI-generated)
test_model_to_netcdf_with_dash_named_parametersintest/test_io.py: themodelfixture plusparameters["my-param"]over a dimensionmy-dim, and a parameter attributemy-attr. Assertsassert_model_equal(comparesparameters) and the parameter attributes. Fails on master'sio.pyunder legacy and v1, passes with the fix.test_model_to_netcdf_with_dash_named_multiindex_dim(legacy only, v1 rejectsMultiIndex): a variable and a parameter over aMultiIndexdimensionmulti-dim. Fails on master'sio.py(theMultiIndexis lost for both), passes with the fix.labels,lower,upper,coeffs,vars,const,sign,rhs, the CSR fields such asindptr,_active_positions) and linopy's own attribute keys contain no dash, socontainer_namesand the variable, constraint, expression and objective paths are unaffected by names with dashes. The existingtest_model_to_netcdf_with_dash_names(x-varnext tox-var-2) covers the longest-prefix choice.xwith a dimensionmi-dimand a variablex-miwith a dimensiondimboth write the attributevariables-x-mi-dim_multiindex. Read assigns it tox-mi. Needs both names in one model; left as is.test/test_io.py,test/test_csr.pyand the other netcdf users (test_sos_reformulation,test_indicator_constraints,test_dtypes,test_scaling,test_piecewise_constraints,test_fix_relax): 1865 passed. The 6 failures areTestSignParameterSOS2 cases intest_piecewise_constraints.py(HiGHS here has no SOS support) and fail identically on master.ruff check,ruff format --checkclean;mypyreports no errors in the changed files (2 existing ones insolvers.pyandremote/oetc.py).parameters["my-param"]next to a parameter carrying a scalar coordmy-scalarlosesmy-scalaron read. Replacing itsprefixes/scalar_ownerblock withds[c].ndim == 0 and owner(str(c)) != prefixinget_prefixfixes that; with it,test/test_io.pyandtest/test_csr.pypass (1033 passed). With both, the aux coord of a dash-named parameter also comes back.fix/netcdf-tz-aware-coords): no textual conflicts, its tz tests and these pass (25 passed, 3 skipped for-k "tz or dash"), and a dash-named parameter and variable over a tz-aware dimensionmy-timecome back withEurope/Amsterdam.🤖 Generated with Claude Code