DM-43332: Improve match_probabilistic performance #188
Conversation
taranu
force-pushed
the
tickets/DM-43332
branch
from
4c70337
to
d8ec2ca
Compare
This way purely astrometric matching will work without an override
This is for the convenience of having reference ra/decs converted to x/y, even if not being used for matching.
| k=config.match_n_max, | ||
| ) | ||
| # Call scipy for non-spherical case | ||
| # The spherical case won't trigger, but the implementation is left for comparison, if needed |
There was a problem hiding this comment.
Do you mean "The non-spherical case won't trigger"?
There was a problem hiding this comment.
How do you anticipate such comparisons becoming necessary?
There was a problem hiding this comment.
No, I mean that the spherical case won't use scipy but will use smatch instead. So the _radec_to_xyz if coords_spherical else np.vstack on line 542 will never actually call _radec_to_xyz.
I anticipate most users will match on spherical coordinates and so the else block won't get called much.
There was a problem hiding this comment.
Ah, I see what you're getting at now. Sorry, that comment is quite confusing in the current version of the code. I mistakenly thought you actually wanted to say "The non-spherical case won't trigger in general" to point out that "The non-spherical case (the else block) won't get called much, and not at all by default."
There was a problem hiding this comment.
So within this else block intended for non-spherical coordinates where coords_spherical is False, there's an if coords_spherical condition that obviously won't be met but might be useful for comparison. I suggest adding a more explicit comment to convey this and clear up any confusion.
| try: | ||
| chisq_sum = np.zeros(n_found, dtype=float) | ||
| chisq_sum[chisq_good] = np.nansum(chi[chisq_good, :] ** 2, axis=1) | ||
| idx_chisq_min = np.nanargmin(chisq_sum / n_finite) |
There was a problem hiding this comment.
Are we always safeguarded against division by zero here?
There was a problem hiding this comment.
As long as n_finite_min > 0, yes.
There was a problem hiding this comment.
Yes, that's why I suggested you add a check in match_n_finite_min = pexConfig.Field(...).
| ) | ||
| match_n_finite_min = pexConfig.Field( | ||
| dtype=int, | ||
| default=3, | ||
| default=2, |
There was a problem hiding this comment.
Maybe add a check here to specify the valid domain?
No description provided.