Skip to content

added maximum threshold#505

Merged
Jammy2211 merged 1 commit into
mainfrom
feature/positions_threshold
May 14, 2026
Merged

added maximum threshold#505
Jammy2211 merged 1 commit into
mainfrom
feature/positions_threshold

Conversation

@NiekWielders
Copy link
Copy Markdown
Collaborator

Summary

added a maximum threshold to the positions_likelihood_from

Changes

only positions_likelihood_from

Testing

no extra tests added

Related Issues

@NiekWielders NiekWielders requested a review from Jammy2211 May 14, 2026 09:16
@Jammy2211
Copy link
Copy Markdown
Collaborator

Thanks @NiekWielders! Before merging I wanted to double-check one specific concern that's bitten us in this area before: the threshold value flowing into min(threshold, maximum_threshold) can be either a numpy scalar or a JAX 0-d array depending on whether the Analysis was constructed with use_jax=True or use_jax=False, and Python's min() can behave differently on the two.

I traced and tested it:

  1. threshold is produced by positions_threshold_from via factor * np.nanmax(positions_fits.max_separation_of_plane_positions). np.nanmax always returns a numpy scalar even when given a concrete JAX array, so threshold arrives here as a numpy scalar (or a Python float in the minimum_threshold early-return branch) regardless of the backend.
  2. I empirically tested min(threshold, maximum_threshold) against numpy scalar, numpy 0-d array, JAX 0-d array (fp32 and fp64), and Python float — all combinations work and return a usable numeric value.
  3. positions_likelihood_from is only called on a Result after the fit completes (i.e. outside any jax.jit boundary), so JAX tracer concretization isn't a concern.

Conclusion: the code is safe for both use_jax=True and use_jax=False. Merging as-is.

Minor style nit (not blocking, just for future reference): the existing minimum_threshold clamp lives inside positions_threshold_from using an if threshold < minimum_threshold: return minimum_threshold pattern, while this PR adds the symmetric maximum_threshold clamp inside positions_likelihood_from using min(). Asymmetric placement but functionally equivalent — happy to leave as-is.

@Jammy2211 Jammy2211 merged commit 6fc18e3 into main May 14, 2026
5 checks passed
@Jammy2211 Jammy2211 deleted the feature/positions_threshold branch May 14, 2026 18:31
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.

2 participants