Skip to content

Conversation

@corentinravoux
Copy link
Owner

NaCl outputs: covariance of 3N x 3N with full covariance for mb, x1 and c parameters (need to convert from x_0 to m_b)

Copilot AI review requested due to automatic review settings January 30, 2026 10:10
@corentinravoux corentinravoux linked an issue Jan 30, 2026 that may be closed by this pull request
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the SN Ia velocity data vector to correctly handle NaCl-style observation covariances that are 3N×3N over the SALT2 parameters (mb, x1, c), instead of an N×N distance-modulus covariance. It introduces a design-matrix–based propagation from the SALT2 parameter covariance to the distance-modulus covariance and then to the velocity covariance.

Changes:

  • Replace the previous direct use of self._covariance_observation with a linear propagation via a design matrix A (and J) so that a 3N×3N SALT2 parameter covariance is mapped into an N×N distance-modulus covariance, including intrinsic scatter via sigma_M.
  • Simplify the velocity covariance computation in give_data_and_variance to apply only the distance-modulus-to-velocity conversion matrix, given that the SALT2-to-distance-modulus propagation is now handled in compute_observed_distance_modulus_variance.
  • Introduce self.number_datapoints to track N, use it consistently in shape checks and identity construction, and adjust _init_A and covariance-shape validation accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 187 to 193
A = jnp.ones((3, self.number_datapoints, 3 * self.number_datapoints))
ij = jnp.ogrid[: self.number_datapoints, : 3 * self.number_datapoints]
for k in range(3):
A[k][ij[1] == 3 * ij[0] + k] = 1
if jax_installed:
A = A.at[k, ij[1] == 3 * ij[0] + k].set(1)
else:
A[k][ij[1] == 3 * ij[0] + k] = 1
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_init_A initializes A with ones and then only ever assigns a subset of entries to 1 again, so the entire matrix stays filled with ones instead of having a single 1 per (SN, parameter) and zeros elsewhere. This makes the derived design matrix J effectively an all-ones matrix and causes the propagated distance-modulus covariance in compute_observed_distance_modulus_variance to be mathematically incorrect; A should be initialized to zeros and only the desired indices set to 1 so that each row selects the appropriate mb/x1/c element for that object.

Copilot uses AI. Check for mistakes.
@corentinravoux corentinravoux changed the title Change to the observation covariance matrix to work with NaCl outputs [bump minor] Change to the observation covariance matrix to work with NaCl outputs Feb 3, 2026
@corentinravoux corentinravoux merged commit 5ed5d25 into main Feb 3, 2026
3 checks passed
@corentinravoux corentinravoux deleted the small_change_cov branch February 3, 2026 16:44
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.

Wrong implementation of _covariance_observation in VelFromSALTfit

1 participant