Skip to content

16 code review - #17

Merged
jacobwilliams merged 18 commits into
masterfrom
16-code-review
Sep 30, 2026
Merged

jacobwilliams merged 18 commits into
masterfrom
16-code-review

Conversation

@jacobwilliams

Copy link
Copy Markdown
Owner

No description provided.

See #16
also add pixi files and other minor changes
Build CSC-style column pointers (col_ptr/col_idx) when the sparsity
pattern is set, and group pointers (grp_ptr/grp_cols) when the
partition is set, instead of scanning the whole pattern for every
column (count/pack over icol) and every group (over ngrp).

* compute_indices now takes n and also builds the column index
  (a stable counting sort, so element order and results are unchanged).
* new compute_group_index, called after dsm, for a user-supplied
  partition, and for the dense partition.
* compute_jacobian_standard, compute_jacobian_partitioned and
  columns_in_partition_group now use index slices; the latter no
  longer grows arrays by concatenation.
* compute_sparsity_dense builds the indices after filling irow/icol.
* set_sparsity_pattern now also rejects irow<1 or icol<1.

Tridiagonal test problem (n=m=20000, central diffs, -O2):
unpartitioned 2.31s -> 0.24s, partitioned 1.75s -> 0.001s.
Test output (jacobians, partitions, function counts) is unchanged.
* the 2-point methods used for sparsity_mode=4 are now looked up once
  in set_sparsity_mode (new sparsity_class_meths component) rather than
  on every compute_sparsity_random_2 call.
* initialize(classes=...) now looks up each distinct class once instead
  of once per variable.
maxlst = sum(row_nnz**2)/n can be 0 for very sparse patterns, in which
case the column selection loop never runs and jcol keeps a stale value.
Use max(1,...) so at least one column is always examined.
On a partial cache hit, the missing functions were computed into the
same f array that held the cached values. Since f is intent(out) in the
user function, the cached values could be lost (e.g., if the user
function initializes f), producing wrong Jacobian elements.

The missing functions are now computed into a temporary array and
merged into f. Results are also no longer cached if the user function
raised an exception.
With sparsity_mode=3, compute_sparsity is null until the user calls
set_sparsity_pattern. compute_jacobian called it unconditionally, so
computing a Jacobian first crashed. It now raises an exception (code 30)
instead, and also checks for exceptions raised while computing the
sparsity pattern.
divide_interval returns fewer points than requested for large
num_points (the top points are clamped to the upper bound and the
duplicates removed), so compute_sparsity_random_2 read past the end of
the coefficient array for num_sparsity_points >~ 164. Zero also crashed.

num_sparsity_points must now be in [1, max_num_sparsity_points] (a new
public constant, 50); other values raise an exception. All points are
distinct and interior up to ~80, so outputs for allowed values are
unchanged. Add divide_interval_test with assertions for the documented
example, n=1..50, and the initialize validation.
The sparsity tolerances were compared absolutely, so functions of small
magnitude (e.g. 1e-20) lost real dependencies, and large ones could
pick up round-off noise.

* equal_within_tol has a new optional `relative` argument.
* sparsity_mode=2 function values and the linear-slope checks now use
  relative comparisons.
* the sparsity_mode=4 zero check now tests whether the change in the
  function (|J|*dx) is within function_precision_tol*|f(x)| at every
  point (the same test as mode 2), instead of |J| against an absolute
  linear_sparsity_tol.
* add sparsity_scaling_test: f, 1e-20*f, and 1e20*f must give the same
  pattern in modes 2 and 4.
The hash table is allocated 0:isize-1, but print looped over
1..size(me%c), skipping slot 0 and reading past the end of the table.
Loop over lbound..ubound instead.

Add cache_test, which prints the cache to a scratch file and checks
that exactly the occupied slots are printed (including slot 0 of a
one-slot cache), plus the uninitialized-cache message.
New diff_test calls diff directly for orders 1-3 over symmetric and
one-sided cases (both directions), x0=0, absolute/relative/zero eps,
relative/absolute accr, very wide and very narrow intervals, and
functions with high-frequency terms. For each case, the actual error
must be within the estimated error and ifail must be consistent with
the requested tolerance.

Also covers invalid inputs (ifail=2), too-small intervals (ifail=3),
constant functions, a kink, the overflow guard, and user termination
at every function evaluation (ifail=-1, no further evaluations, next
call unaffected). The only uncovered statement is unreachable in
double precision.
@codecov

codecov Bot commented Sep 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.62874% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.08%. Comparing base (07a4ad6) to head (7779e4b).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #17       +/-   ##
===========================================
+ Coverage   66.30%   86.08%   +19.78%     
===========================================
  Files           5        5               
  Lines        1938     2027       +89     
===========================================
+ Hits         1285     1745      +460     
+ Misses        653      282      -371     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The DSM partition was computed from the nonlinear pattern only, so
columns sharing a row through a linear (constant) element could be
grouped and perturbed together, silently corrupting the nonlinear
elements in that row.

* dsm_wrapper now partitions on the nonlinear + linear pattern.
* set_sparsity_pattern stores the linear pattern before partitioning.
* a user-supplied partition is checked for consistency against the
  full pattern (new exception 33).
* linear indices < 1 are now rejected.

Add linear_partition_test: checks the partitioned jacobian against the
analytic one (forward, central, class 3), that it is identical to the
unpartitioned jacobian, and that inconsistent user partitions are
rejected.
dfunc called the numdiff_type terminate (a no-op, since the exception
was already raised) instead of the diff_func one, so diff kept
evaluating the function after a user termination (and with the cache,
the termination status was replaced by a DIFF error). It also did not
check for a termination from the info function before evaluating the
function.

Add terminate_test: terminates at every function evaluation and every
info call for diff and finite difference modes (including during the
sparsity computation), and checks that no further evaluations occur.
A chunk_size of 0 made expand_vector allocate a size-0 array and then
write past its end. expand_vector now grows by max(1,chunk_size), and
numdiff_type and function_cache store max(1,...) as well.

Add chunk_size_test: chunk sizes 0, -3, 1, 5, 100 through
expand_vector, unique, the function cache, and a sparsity computation.
options_test covers jacobian_methods/classes, perturb modes 1-3 and
set_dpert (checking the actual steps), class method selection at the
bounds (with/without partitioning, including when no method fits),
sparsity bounds and dpert_for_sparsity, diff outside the bounds, and
that the cache does not change the results (including a partial hit).

validation_test covers each reachable input error code, error status,
formulas, printing a method, and returning/printing the sparsity
pattern with linear elements and a partition.

Fix: with partitioning enabled, a function that does not depend on x
(empty sparsity pattern) raised an exception, since dsm rejects an empty
pattern. All columns are now put in one group.
@jacobwilliams
jacobwilliams merged commit 612e02f into master Sep 30, 2026
1 check passed
@jacobwilliams
jacobwilliams deleted the 16-code-review branch September 30, 2026 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant