Stabilize post-nalgebra gates and canonicalize backend module names#5
Conversation
Greptile OverviewGreptile Summarycanonicalized backend module naming so nalgebra (default) uses unsuffixed file names and ndarray compatibility uses Key Changes:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| .github/workflows/rust.yml | added strict-gates job with clippy -D warnings, fmt check, and py-bindings check |
| crates/xraytsubaki/src/xafs/mod.rs | reversed module path logic: nalgebra (default) uses unsuffixed names, ndarray uses *_ndarray suffixed paths |
| crates/xraytsubaki/src/xafs/mathutils.rs | now contains nalgebra implementation (was ndarray), removed ptp/argmin/argmax trait methods |
| crates/xraytsubaki/src/xafs/mathutils_ndarray.rs | moved from mathutils_nalgebra.rs, contains full ndarray implementation with all trait methods |
| crates/xraytsubaki/src/xafs/background.rs | migrated to nalgebra types (DVector/DMatrix), removed ndarray imports, added dvector_arange helper |
| crates/xraytsubaki/src/xafs/xafsutils.rs | refactored to nalgebra backend, removed ndarray impl, fixed early return in etok, cleaned up docs |
| py-xraytsubaki/src/lib.rs | added #![allow(non_local_definitions)], removed nested Option check for chir_mag |
Sequence Diagram
sequenceDiagram
participant CI as GitHub Actions
participant Core as Core Tests
participant Gates as Strict Gates
participant Build as Build System
CI->>Core: Run cargo test -p xraytsubaki
Core-->>CI: Tests pass
CI->>Gates: Trigger strict-gates job
Gates->>Build: cargo test -p xraytsubaki
Build-->>Gates: Tests complete
Gates->>Build: cargo clippy -p xraytsubaki --all-targets -- -D warnings
Note over Gates,Build: Enforce zero warnings policy
Build-->>Gates: No warnings
Gates->>Build: cargo fmt --all -- --check
Note over Gates,Build: Verify code formatting
Build-->>Gates: Formatting correct
Gates->>Build: cargo check --manifest-path py-xraytsubaki/Cargo.toml
Note over Gates,Build: Validate Python bindings
Build-->>Gates: Bindings valid
Gates-->>CI: All gates passed
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the post-nalgebra baseline by enforcing strict CI quality gates, fixing build issues (notably Python bindings), and canonicalizing backend module/file naming so the nalgebra-default path uses unsuffixed module names while ndarray-compat uses *_ndarray.rs sources.
Changes:
- Add/enforce strict CI gates (tests, clippy
-D warnings, fmt check, Python bindingcargo check) and document them in OpenSpec + migration notes. - Canonicalize backend module file names and
cfgrouting for default vsndarray-compatbuilds. - Refactor FFT/mathutils/normalization/background compatibility implementations and adjust bindings/tests to satisfy strict gate requirements.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| py-xraytsubaki/src/xasspectrum.rs | Removes unused imports in Python binding wrapper module. |
| py-xraytsubaki/src/lib.rs | Adjusts Python pipeline output building; adds crate-level lint allow. |
| openspec/changes/update-post-nalgebra-stability-gates/tasks.md | Adds strict-gate task checklist. |
| openspec/changes/update-post-nalgebra-stability-gates/specs/repo-integration-support/spec.md | Specifies Python binding build/contract requirements. |
| openspec/changes/update-post-nalgebra-stability-gates/specs/build-and-baseline-gating/spec.md | Specifies strict core/compat/CI parity requirements. |
| openspec/changes/update-post-nalgebra-stability-gates/proposal.md | Proposal describing why/what/scope of strict gates. |
| openspec/changes/update-post-nalgebra-stability-gates/design.md | Design doc for stabilizing post-migration baseline and CI parity. |
| openspec/changes/refactor-canonicalize-backend-module-names/tasks.md | Task checklist for backend file renames and routing updates. |
| openspec/changes/refactor-canonicalize-backend-module-names/specs/xas-core/spec.md | Specifies naming expectations for default vs ndarray compatibility modules. |
| openspec/changes/refactor-canonicalize-backend-module-names/proposal.md | Proposal describing the canonicalization refactor. |
| openspec/changes/refactor-canonicalize-backend-module-names/design.md | Design doc describing the rename strategy and risks. |
| crates/xraytsubaki/src/xafs/xrayfft_ndarray.rs | Introduces ndarray-compat FFT implementation (and tests) under new naming. |
| crates/xraytsubaki/src/xafs/xrayfft_nalgebra.rs | Removes old nalgebra-suffixed FFT source in favor of canonical naming. |
| crates/xraytsubaki/src/xafs/xrayfft.rs | Makes canonical FFT module nalgebra-first and trims ndarray-specific code. |
| crates/xraytsubaki/src/xafs/xasspectrum.rs | Formatting/cleanup; uses unwrap_or_default() for FFT config; test formatting tweaks. |
| crates/xraytsubaki/src/xafs/xasparameters.rs | Avoids unused variable warning in test. |
| crates/xraytsubaki/src/xafs/xasgroup.rs | Tightens tests (unwrap fallible removal) and removes unused locals. |
| crates/xraytsubaki/src/xafs/xafsutils_nalgebra.rs | Removes old nalgebra-suffixed utils source in favor of canonical naming. |
| crates/xraytsubaki/src/xafs/normalization_ndarray.rs | Updates ndarray-compat normalization types/logic and docs. |
| crates/xraytsubaki/src/xafs/normalization.rs | Restores canonical normalization module to nalgebra-first types/behavior. |
| crates/xraytsubaki/src/xafs/mod.rs | Updates cfg routing to canonical module filenames and *_ndarray paths. |
| crates/xraytsubaki/src/xafs/mathutils_ndarray.rs | Introduces ndarray-compat mathutils under *_ndarray naming. |
| crates/xraytsubaki/src/xafs/mathutils_nalgebra.rs | Removes old nalgebra-suffixed mathutils source. |
| crates/xraytsubaki/src/xafs/mathutils.rs | Canonical mathutils becomes nalgebra-first; removes ndarray implementations. |
| crates/xraytsubaki/src/xafs/lmutils.rs | Minor numeric constant/format cleanup for lint friendliness. |
| crates/xraytsubaki/src/xafs/io/xafs_json.rs | Uses f64::EPSILON shorthand in tests. |
| crates/xraytsubaki/src/xafs/io/xafs_bson.rs | Uses f64::EPSILON shorthand in tests. |
| crates/xraytsubaki/src/xafs/background_ndarray.rs | Updates ndarray-compat background implementation to ndarray storage/views and new routing. |
| crates/xraytsubaki/src/xafs/background.rs | Canonical background module remains nalgebra-first; aligns types and minor lint annotations. |
| crates/xraytsubaki/examples/alloc_baseline.rs | Avoids unnecessary to_string() when loading spectrum paths. |
| crates/xraytsubaki/doc/migration-performance-logic-hardening.md | Documents strict post-nalgebra gate commands and baseline failures. |
| crates/xraytsubaki/benches/autobk_stage_benchmark.rs | Formatting-only change for profiler flag detection. |
| crates/xraytsubaki/Cargo.toml | Moves bench profile config out of crate manifest. |
| ERROR_HANDLING_MIGRATION.md | Updates module filenames in migration checklist to canonical names. |
| Cargo.toml | Moves [profile.bench] to workspace root; removes workspace.package name. |
| .github/workflows/rust.yml | Adds strict-gates CI job and makes benchmark depend on it. |
Comments suppressed due to low confidence (3)
crates/xraytsubaki/src/xafs/normalization_ndarray.rs:595
MBack::fill_parameter()currently usestodo!(), which will panic at runtime if this path is hit (e.g., viaNormalizationMethod::fill_parameter). Please make this a no-op (as innormalization.rs) or return a structuredNormalizationError::NotImplementedinstead of panicking.
crates/xraytsubaki/src/xafs/normalization_ndarray.rs:20- Doc comment contains a typo: “impliments” should be “implements”.
crates/xraytsubaki/src/xafs/normalization_ndarray.rs:20 - Doc comment contains a typo: “nomalization” should be “normalization”.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn xftf( | ||
| &mut self, | ||
| k: ArrayBase<ViewRepr<&f64>, Ix1>, | ||
| chi: ArrayBase<ViewRepr<&f64>, Ix1>, | ||
| ) -> Result<&mut Self, FFTError> { | ||
| let (cchi, win) = self.xftf_prep(k, chi)?; |
There was a problem hiding this comment.
XrayFFTF::xftf/xftf_prep assumes k.len() >= 2 (indexes k[1]) and non-empty inputs (unwraps max_by(...)). With short/empty arrays this will panic instead of returning an FFTError.
Add explicit validation in xftf (and/or xftf_prep) for length mismatch and len() < 2, returning FFTError::InsufficientPoints (and a clear interpolation/window error for other invalid inputs).
There was a problem hiding this comment.
Fixed in 43ceb1b. Added explicit guards in xrayfft_ndarray::xftf_prep for k/chi length mismatch and len < 2, returning FFTError (InterpolationFailed or InsufficientPoints) instead of panicking on k[1]/max_by unwrap paths.
| #![allow(non_local_definitions)] | ||
|
|
There was a problem hiding this comment.
Crate-level #![allow(non_local_definitions)] is very broad and can mask unrelated issues. Prefer scoping this to the specific item(s) triggering the lint (e.g., a #[allow(non_local_definitions)] on the affected #[pymodule]/#[pyfunction] block) or upgrading/configuring the offending macro usage so the allow isn’t needed.
There was a problem hiding this comment.
Addressed in 43ceb1b. Removed the broad crate-level allow from py-xraytsubaki/src/lib.rs and scoped non_local_definitions allowance to py-xraytsubaki/src/xasgroup.rs.
| strict-gates: | ||
| name: Strict Gates (stable) | ||
| runs-on: ubuntu-latest | ||
| needs: core | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Setup Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| toolchain: stable | ||
| components: clippy, rustfmt | ||
| - name: Run core crate tests | ||
| run: cargo test -p xraytsubaki --verbose | ||
| - name: Run strict clippy | ||
| run: cargo clippy -p xraytsubaki --all-targets -- -D warnings | ||
| - name: Check formatting | ||
| run: cargo fmt --all -- --check | ||
| - name: Check Python bindings crate | ||
| run: cargo check --manifest-path py-xraytsubaki/Cargo.toml --verbose |
There was a problem hiding this comment.
strict-gates re-runs cargo test even though the core job already runs tests (stable+beta), and it also waits on the entire matrix via needs: core. This increases CI time and delays clippy/fmt feedback.
Consider either: (1) dropping the duplicate cargo test from strict-gates, or (2) making strict-gates independent / depend only on the stable core leg, or (3) folding strict checks into the stable core job.
There was a problem hiding this comment.
Applied in 43ceb1b. strict-gates no longer re-runs cargo test and no longer has needs: core, so clippy/fmt feedback starts earlier and avoids duplicate test execution.
Greptile OverviewGreptile SummaryThis PR completes the nalgebra migration stabilization by enforcing strict quality gates and canonicalizing backend module naming. Key Changes
ImpactThe refactoring makes the codebase naming align with runtime reality (nalgebra is default). Both backends remain functional through feature flags, with strict quality enforcement preventing regressions. Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| .github/workflows/rust.yml | Added new strict-gates job enforcing clippy with -D warnings, rustfmt check, and Python bindings check |
| crates/xraytsubaki/src/xafs/mod.rs | Inverted module path configuration so unsuffixed files are default (nalgebra), suffixed with _ndarray for compatibility mode |
| crates/xraytsubaki/src/xafs/background.rs | Migrated from ndarray to nalgebra types (DVector), removed ndarray-specific view methods, added clippy allow for large_enum_variant |
| crates/xraytsubaki/src/xafs/mathutils.rs | Consolidated nalgebra-based math utilities, removed ndarray implementations, used saturating_sub for safer length arithmetic |
| crates/xraytsubaki/src/xafs/xafsutils.rs | Migrated to nalgebra DVector types, improved error handling with Result returns, removed ndarray trait implementations |
| crates/xraytsubaki/src/xafs/xrayfft.rs | Migrated FFT implementation from ndarray to nalgebra, consolidated default backend |
| py-xraytsubaki/src/lib.rs | Fixed optional chir_mag handling by removing unnecessary if-let guard |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant CI as CI Pipeline
participant Core as Core Tests
participant Gates as Strict Gates
participant Bench as Benchmarks
Dev->>CI: Push commit
par Core Job
CI->>Core: cargo check -p xraytsubaki
CI->>Core: cargo test -p xraytsubaki
Core-->>CI: Tests pass
and Strict Gates Job
CI->>Gates: cargo clippy -p xraytsubaki --all-targets -- -D warnings
Gates-->>CI: No warnings
CI->>Gates: cargo fmt --all -- --check
Gates-->>CI: Formatting correct
CI->>Gates: cargo check py-xraytsubaki
Gates-->>CI: Python bindings compile
end
Core->>Bench: Core tests complete
Gates->>Bench: Strict gates complete
CI->>Bench: cargo bench
Bench-->>CI: Benchmarks complete
CI-->>Dev: All checks pass ✓
Note over Dev,Bench: Module path routing
alt Default build (nalgebra)
Dev->>Core: cargo check
Core->>Core: Load background.rs
Core->>Core: Load mathutils.rs
Core->>Core: Load xafsutils.rs
Core->>Core: Load xrayfft.rs
else ndarray-compat feature
Dev->>Core: cargo check --features ndarray-compat
Core->>Core: Load background_ndarray.rs
Core->>Core: Load mathutils_ndarray.rs
Core->>Core: Load xafsutils_ndarray.rs
Core->>Core: Load xrayfft_ndarray.rs
end
|
Addressed review feedback in commit
Validation rerun after changes:
|
Summary
Validation