Repository navigation
Jacobian error - #1202
Merged
Merged
Jacobian error#1202
Conversation
spectral_rad_jacAddSensorJacobianPerturbations wrote the sensor derivative to row target_pos of spectral_rad_jac, but that matrix is indexed by model state element, not by target. With any other target before the sensor target, the derivative landed in an unrelated column (e.g., temperature at level 1) and the sensor column stayed zero. The existing test only had sensor targets at polyorder 0, where the two indices coincide. The model state of a sensor target is the coefficients of a polynomial offset of the grid. Write the derivative to x_start + k for each coefficient k, scaled by d(grid)/d(c_k) from the target's polynomial model. Monochromatic radiance only depends on its own frequency, so the single shifted-grid calculation already gives the derivative w.r.t. every frequency independently and no extra agenda calls are needed. Previously, only the constant term got a derivative. Add a test placing a temperature target before the sensor target, and checking the linear term against a brute-force finite difference. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Targets::finalized was set by finalize() but never cleared, so adding a target after jac_targetsFinalize, or calling jac_targetsInit and adding targets again, left the flag set. x_size() then summed the unassigned max() sentinels and silently returned a wrapped-around size (e.g., 49 instead of 100, or 2^64 - 1). Reset the flag in clear(), emplace_back() and the overlapping wind and magnetic field adders. The const-reference emplace_back overloads now forward to the rvalue ones, so the reset lives in one place per type. The target lists are writable from Python, which bypasses the flag, so x_size() now also requires every target to have been positioned. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Overlapping targets share one range of the model state, e.g., the magnitude of the wind or magnetic field as represented by its three components. Several things went wrong once such a group was transformed to relative, logarithmic, etc.: - update_jac applied the inverse Jacobian once per member to the same columns, so a relative magnetic field was scaled by orig^3. - jac_targetsAddOverlappingWindField moved from its copy of the original and then reused it, so the third component lost the transformation and update_model wrote the raw model state into its field. - Toggling a transformation after adding the overlaps (the usual order, as toggles come after jac_targetsFinalize) only changed the original. Forbid transformations on overlapping targets and their originals in the toggles, in the overlap adders, and in finalize. Only the original target updates the shared model state and Jacobian columns; every member still updates its own field. Overlap only makes sense when the components are magnitude fields, where the summed columns are the derivative w.r.t. the magnitude, so finalize now requires that. Previously, plain component fields were silently set to u = v = w = x. The overlap adders now share one helper that builds fresh targets instead of copying the original, and the is_wind and is_mag helpers they used are removed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Saving jac_targets failed for most transformations, as only the atmospheric functors were registered with xml_io_stream_functional and that list was incomplete: - log-relative and relative humidity atmospheric targets failed with "Unknown function struct", - any transformed surface or subsurface target, every sensor polynomial offset, and every error polynomial fit failed with "Cannot write function structs". Register the functors of all target types. The new atmospheric entries are appended to keep the existing variant order. The relative humidity functors hold the saturation pressure operator, which was a lambda and could not be written at all. Replace the two lambdas of water_equivalent_pressure_operatorMK05 with a WaterEquivalentPressureMK05 functor that is registered for NumericUnaryOperator. This also makes water_equivalent_pressure_operator itself saveable. User-defined operators, e.g., from Python, still cannot be written. Add a test that round-trips all transformation types through ascii and binary XML and compares the read targets with the originals. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Several functors reported themselves as another one, e.g., the relative
and log-relative functors all said "polyinv", rhinv said "rhfwd", and
the polynomial fit and inverse had their names swapped. The log-relative
size errors said "relative profile", and ErrorTarget::update_model and
update_state complained about the "Measurement Jacobian", which they do
not touch.
Also remove the commented-out ARTS2 variant of the relative inverse
Jacobian, which is the same as the one in use (y / x == orig), and the
no-op catch (...) { throw; } clauses in the relative humidity functors.
No change in behaviour.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b5d0abb ("Allow unused LineByLineVariable") flipped the test on ls_var from != to ==. Line parameter targets (ls_var unused) were looked up among the line shape models and failed with "Missing data", and line shape targets fell through to the line parameter switch and failed with "Invalid operation". So no line target could be moved between abs_bands and the model state, i.e., model_state_vecFromBands and abs_bandsFromModelState always failed. Add a test round-tripping an f0 and a G0 X0 target. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Relative humidity targets with nonnegative=1 clamp the inverse state to zero for negative model state elements, but the inverse Jacobian still scaled those columns by psat/p. The field does not depend on those elements, so zero their columns instead. Targets::finalize checked every target type for duplicates except error targets, so two identical error targets were accepted. Check them too. Targets::finalize took abs_bands but ignored it, so a line target for a missing band, line, or line shape species was only reported once the model state was used. Check every line target against abs_bands. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
target_comparable and valid_target are not used anywhere. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
xml_io_stream<std::function> identifies a registered function pointer by comparing the address of the pointer stored in the std::function with the address of the registered pointer. These are never the same object, so writing a registered function pointer always failed with "Unknown function pointer". Compare the pointers themselves instead. Nothing registers function pointers yet, so add a C++ test that does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Most of Targets repeated the same code once per target type. Add a for_each_list helper that calls a function on each of the six lists in model state order, and use it in clear, x_size, target_count, and model_state_vecPerturbations. In finalize, the six copies of the duplicate and positioning loops become one each. The positioning loop takes the size of a target from a per-type function, and the duplicate check uses a per-type comparison, where sensor targets are still the same if they share the relevant grid. The duplicate sensor target error no longer has its own note, otherwise no change in behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The relative, logarithmic, log-relative and relative humidity toggles lived in four files with ten near-identical implementations. Replace them with one toggle<Inv> helper in m_jactargets_toggle.cc, so each workspace method is a single call. No change in behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The relative, logarithmic and log-relative functors had one overload of each call operator per field type, and one make_*fit per target type, all identical apart from the types. Replace them with one call operator constrained on the new Jacobian::transformable_field concept and one templated make_*fit each. jacobian_names.h gets the include guard it was missing, and loginv now says "logarithmic profile" in its size errors instead of "relative". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The six std::formatter specializations for the target types were identical apart from the type. Replace them with one constrained on Jacobian::target_type. Error targets now also print their perturbation, like the others. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The line targets in model_state_atm.py were commented out and used an old interface. Add f0, a, and every G0 and Y coefficient for O2 and AIR with the current methods. They now pass through the model state since the line_key::get_value fix. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
x_size() checks that every target has been positioned, which catches targets appended to the lists from Python after finalizing. The covariance and OEM code tested the bare finalized flag and missed that. Move the check into Targets::is_finalized and use it in x_size, in the covariance methods, in oemRestoreApriori, and in the OEM data validation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Add a relative subsurface temperature target to the XML round trip, so that the subsurface transformation functors are written and read too. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The polynomial offset of a sensor grid used the raw grid as abscissa, i.e., frequency in Hz. Coefficient k then scaled with f^k, about 1e11 per order, so retrievals with a polynomial order above 0 were badly conditioned and their priors hard to set. Map the grid to [-1, 1] before evaluating and fitting the polynomial, so every coefficient is a frequency shift in Hz: c0 at the centre of the grid and c0 +- c1 at its edges. The grid is mapped on the fly from the stored original grid, so the XML format is unchanged. Order 0 is unchanged. Also reject polynomial orders that the number of grid points cannot determine when the target is added. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
c4f7730 removed Jacobian::is_wind, which m_lookup.cc still used. Atm::is_wind checks the same keys. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Jacobian commits were written against the ARTS_USER_ERROR_IF macro, which main has since removed in favour of `if (...) arts::user_error(...)`. Convert the uses they added. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Targets::is_finalized scanned every target for an unassigned position to catch targets appended to the lists from Python, which bypasses the finalized flag. Checking finality should be free. Remember the number of targets in finalize instead, and compare it in is_finalized. Any target added after finalize, however it was added, still requires finalizing again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
Jacobian targets: bug fixes, validation and cleanup
This PR fixes several bugs in how Jacobian targets map between fields, the model state vector and the measurement Jacobian. It also adds checks that report misconfigured targets when they are set up, not mid-retrieval, and reduces the duplicated code in
src/core/jacobianby about 750 lines.Bug fixes
Sensor frequency offset Jacobian landed in the wrong column.
spectral_rad_jacAddSensorJacobianPerturbationsindexed an x-sized matrix bytarget_pos, notx_start. With any target before the sensor target, the derivative went into an unrelated column (e.g. temperature at level 1), and the sensor column stayed zero. The existing test only used sensor targets at polyorder 0, where the two indices coincide. Higher polynomial orders now get their derivatives as well, from the same single perturbation.Line targets could not use the model state at all.
line_key::get_valuehad an inverted check (introduced in b5d0abb), somodel_state_vecFromBandsandabs_bandsFromModelStatefailed for every line parameter and line-shape target.Stale
finalizedflag. Adding targets afterjac_targetsFinalize, or afterjac_targetsInit, left the flag set, andx_size()returned wrapped-around sizes (e.g. 2^64 − 1). The newTargets::is_finalized()also catches targets appended from Python, and the covariance and OEM code now use it.Overlapping targets with transformations (relative, log, …):
jac_targetsAddOverlappingWindFieldmoved from a copy and then reused it, so one component lost its transformation;Transformations on overlap groups are now rejected. Only the original target updates the shared state and Jacobian columns.
XML serialization of transformed targets. Only some atmospheric functors were registered. Log-relative and relative-humidity targets, any transformed surface or subsurface target, and every sensor or error polynomial target failed to save. All are now registered. The MK05 saturation-pressure operator is now a serializable functor, so
water_equivalent_pressure_operatoritself can also be saved.Registered function pointers in functional XML IO were matched by the address where they were stored, so they could never be written. Nothing used this path yet.
RH targets with
nonnegative=1kept a non-zero Jacobian where the state is clamped to zero.Behaviour changes
jac_targetsFinalizenow:abs_bands;atm_fieldAbsolute*Field).dlike the other target types. The duplicate-sensor-target error no longer carries its own note.Cleanup
Targets::for_each_listreplaces six copies of most per-type loops, including those infinalize.m_jactargets_*toggle.ccfiles are merged into onem_jactargets_toggle.ccwith a single templated helper.transformable_fieldconcept, and one templatedmake_*fiteach. Before, there was one per field type.std::formatterfor all target types replaces six identical ones.target_comparableandvalid_targetand the redundantJacobian::is_wind/is_magare removed.Tests
New:
tests/core/jac/:jac_targets_finalize.pyoverlapping_targets.pytargets_xml_round_trip.pyline_targets_model_state.pyjac_targets_validation.pyrh_nonnegative_jacobian.pytests/core/sensor/sensor_poly_offset_jacobian.pycpp.fast.xml_functionalExtended:
cpp.fast.covariance.workspace. The line targets intests/core/jac/model_state_atm.pyare re-enabled.Notes for review
ARTS_USER_ERROR_IFmacro and rebased onto the change that removed it. The last commit converts the remaining uses, so the intermediate commits do not compile on their own. Squash-merging avoids that.🤖 Generated with Claude Code