DM-43332: Add support for spherical coord distances in DiffMatchedTractCatalogTask #929
Conversation
| dist[matched_row] = sphdist( | ||
| target_match_c1, target_match_c2, target_ref_c1, target_ref_c2 | ||
| ) if config.coord_format.coords_spherical else np.hypot( | ||
| target_match_c1 - target_ref_c1, target_match_c2 - target_ref_c2, | ||
| ) | ||
| # Should probably explicitly add cosine terms if ref has errors too | ||
| dist_err[matched_row] = sphdist( | ||
| target_match_c1, target_match_c2, | ||
| target_match_c1 + cat_target.iloc[matched_row][coord1_target_err].values, | ||
| target_match_c2 + cat_target.iloc[matched_row][coord2_target_err].values, | ||
| ) if config.coord_format.coords_spherical else np.hypot( |
There was a problem hiding this comment.
Doesn't this need to be tested in tests/test_diff_matched_tract_catalog.py?
There was a problem hiding this comment.
It is, but you raise a good point that I have to update the reference data because the spherical difference are different from the old hypot.
Just to confirm, only the distances change, i.e.:
{k: v for k, v in dict(result.diff_matched.iloc[0] - self.diff_matched).items() if 'dist' not in k and (v != 0)} prints nothing.
There was a problem hiding this comment.
I will add a second part to the test to set that config value to the opposite of the default and run again. Having said that, we don't generally test every single config permutation in tasks; it's only viable here since it's a boolean.
| @@ -44,7 +44,8 @@ | |||
| import numpy as np | |||
| import pandas as pd | |||
| from scipy.stats import iqr | |||
| from typing import Dict, Sequence | |||
| from smatch.matcher import sphdist | |||
| from typing import Sequence | |||
There was a problem hiding this comment.
Now that we're getting rid of the Dict import from typing on this ticket, we should tidy up the docstrings in a couple of places: compute_stats() and _get_columns().
There was a problem hiding this comment.
That part of the code is slated for deprecation in RFC-1008 and so I'm not updating any of it.
| ) if config.coord_format.coords_spherical else np.hypot( | ||
| target_match_c1 - target_ref_c1, target_match_c2 - target_ref_c2, | ||
| ) | ||
| # Should probably explicitly add cosine terms if ref has errors too |
There was a problem hiding this comment.
Are we ignoring the errors in the ref catalog because they're so tiny they don't really matter, making it overkill? Or are you thinking we should start paying attention to them sometime soon?
There was a problem hiding this comment.
Right now this is only used for DC2 and source injection, where the reference catalogs are truth with no errors. It will take a lot more work on a separate ticket to incorporate reference catalog errors.
There was a problem hiding this comment.
Ok, I understand. If it's something important that needs to be addressed soon, I'd create a separate ticket for it now and drop the ticket number here as a TODO.
This is temporary and only necessary due to a bug in smatch 0.10.1 in the sphdist function.
No description provided.