Refactor explicit dataflow setup tables#82
Conversation
…factor Update src/AGENTS.md source tree to the rtm/optics/spectrum/setup/cache layout and document the explicit-dataflow rule (banned god-object identifiers, allowed named data owners, *const reads / single named write target) enforced by scripts/linting/check-explicit-dataflow-signatures.py. Replace the STYLEGUIDE.md function-map example that taught the now-banned Workspace naming with a solveReflectance example using *TransportWorkArrays, and add a dataflow section to the recognized function-map sections.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 307ed9f4cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc5cb3255e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89407e5788
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bout3fiddy
force-pushed
the
codex/explicit-dataflow-refactor
branch
2 times, most recently
from
59c12ea to
abe782f
Compare
bout3fiddy
force-pushed
the
codex/explicit-dataflow-refactor
branch
2 times, most recently
from
0191dcd to
cffbd37
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cffbd37. Configure here.
bout3fiddy
force-pushed
the
codex/explicit-dataflow-refactor
branch
from
cffbd37 to
91d8b0b
Compare
There was a problem hiding this comment.
💡 Codex Review
zdisamar/python/zdisamar/input/shared.py
Lines 101 to 102 in 91d8b0b
Emit native placeholders as parseable nulls
When Python-built scenes are sent through Scene.to_native_json_bytes(), the default altitude placeholders are math.nan, and this conversion writes them as the JSON string "nan". The native parser only normalizes bare NaN tokens and its placeholder fields are typed as ?f64, so normal scenes from o2a.reference_scene() or validation.o2a.case.build_o2a_case() fail during zds_prepare_o2a_json before the inert-placeholder validation can accept them; emit null (or leave bare NaN for the existing normalizer) instead of a string.
Keep diagnosis batch workers at the native-supported count
Fresh evidence after the native rejection is that diagnose_retrieval() still calls this helper when batch_workers is omitted; with the default 30 starts on any machine where ZDISAMAR_WORKER_LIMIT/CPU count is at least 2, it returns 2. That value is passed through diagnosis_batch() to the C batch request, which now rejects batch_worker_count != 1, so a default Result.diagnose() fails before running the multi-start sweep; clamp this helper/default to 1 or expose a supported Python-level parallel path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (source_interval_index > 0) { | ||
| source_interval_index -= 1; | ||
| const interval = intervals[source_interval_index]; | ||
| const rtm_interval_index_1based: u32 = @intCast(intervals.len - source_interval_index); |
There was a problem hiding this comment.
When callers place scalar aerosol or the aerosol-pressure retrieval state in interval 1 or 3, the scene validates aerosol.interval_index_1based against the input interval ids, but this layer-grid fill renumbers intervals bottom-up with intervals.len - source_interval_index. fillSupportOpticsAtWavelength later matches aerosol support rows by these rewritten ids, so valid aerosol optical depth is deposited into the opposite interval instead of the requested pressure span. Preserve interval.index_1based here, or remap the aerosol table consistently, so enabled aerosol is not moved silently.
Useful? React with 👍 / 👎.
| const strong_state = line_physics.prepareStrongLineState( | ||
| tables.lines.strong_lines, | ||
| tables.lines.relaxation_matrix, |
There was a problem hiding this comment.
When isotopes_sim excludes isotope 1, the forward profile-line path now disables the LISA strong-line sidecars, but this diagnostic builder still prepares sidecar state from the unfiltered tables.lines.strong_lines and then emits those rows. That makes o2_line_contributions report isotope-1 strong/line-mixing contributions for an isotope-2-only run, contradicting the spectrum and misleading validation diagnostics; apply the same isotope-one gate used by the profile-line cache before preparing these rows.
Useful? React with 👍 / 👎.
| self._scene = copy.deepcopy(rtm_scene) if copy_scene else None | ||
| self._scene_fingerprint = payload | ||
| self._loaded_has_multi_layer_aerosol_profile = len(rtm_scene.aerosol.profile) > 1 | ||
| self._solar_mu0 = rtm_scene.geometry.solar_mu0 |
There was a problem hiding this comment.
For near-horizon solar angles, the native ABI now converts reflectance Jacobians back to radiance with the floored transport cosine, but this handle stores the raw cos(sza) and Spectrum.reflectance_jacobian() divides by that value. Fresh evidence after the native fix is that compactRadianceJacobian uses the floor while this saved solar_mu0 does not, so Python callers get reflectance Jacobians inflated by 0.05 / cos(sza) for accepted grazing geometries; store/expose the same floored cosine here (and when include_scene=True).
Useful? React with 👍 / 👎.
| if (!has_strong_lines) return @max(instrument.adaptive_points_per_fwhm, 1); | ||
| const min_divisions = @max(instrument.strong_line_min_divisions, 1); | ||
| const max_divisions = @max(instrument.strong_line_max_divisions, min_divisions); |
There was a problem hiding this comment.
When native JSON or a typed Scene sets adaptive_reference_grid.points_per_fwhm, strong_line_min_divisions, or strong_line_max_divisions to 0 (or a max below min), validation still accepts the case and this hot path coerces the parsed values with @max(..., 1)/@max(max, min). That silently changes an experiment control and produces a support grid different from the requested one; reject zero or inconsistent adaptive-grid counts at the input boundary instead.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Note
High Risk
Large-scale move of RTM/optics/spectrum code and a breaking Python API rename touch the core forward and retrieval path; benchmark retrieval skip logic also means OE coverage can be absent when native retrieval is still unsupported.
Overview
This PR restructures the Zig product around an explicit pipeline (
input→setup→optics→rtm→spectrum→output/retrieval) instead of the oldforward_modellayout, with hot-path rules documented insrc/AGENTS.mdand STYLEGUIDE (dataflowsections, banned god-object parameter names).Build and tests are realigned: instrumentation stubs move under
src/instrumentation/,zig builddrops the old validation-o2a step matrix in favor oftest-unit/test-rtm/test-retainedand C ABI gates, and benchmarks can skip OE when retrieval raises typed “unsupported” errors instead of failing the whole run.Python renames the O2 A surface:
O2AInput→Scene,reference_case()→reference_scene(), RTMload_scene, OEscene=/scene_for_state, and related type renames (LineByLine,Optimisation). JSON parsing gainsplaceholder_floatfor omitted/null optional fields;SurfaceAlbedois no longer in the public OE exports.Docs, DISAMAR link targets, and regenerated benchmark/results.json follow the new paths and naming.
Reviewed by Cursor Bugbot for commit 91d8b0b. Bugbot is set up for automated code reviews on this repo. Configure here.