Feature/hr diagram - #128
Conversation
…correcting an issue in the light curve nearest target computation
| return distance_min, distance_max | ||
|
|
||
|
|
||
| def estimate_membership(pmra, pmdec, parallax, distance=None, distance_lo=None, distance_hi=None): |
There was a problem hiding this comment.
No reason for these distances to be optional. Every caller supplies all 3.
| clamped non-negative since Bailer-Jones distances are always positive. | ||
| """ | ||
| if distance is None: | ||
| return None, None |
There was a problem hiding this comment.
See below - how can distance be none? What is this checking for?
| # well behaved even where the parallax is negative or low-significance. | ||
| # r_med_geo is the point estimate (parsecs); r_lo_geo / r_hi_geo are the 16th / 84th | ||
| # percentiles (asymmetric, 1-sigma-like bounds). source_ids match gaiadr3.gaia_source. | ||
| # Photogeometric columns (r_med_photogeo,...) are sharper where BP/RP photometry is good but bake in a Galactic CMD population model; |
There was a problem hiding this comment.
I think this is one of those claude comments that over explain and include decisions taken along the way that are not relevant to future readers?
| gaia_data = gaia_cone_search(cluster_ra, cluster_dec, search_radius_arcmin) | ||
| self._report_progress('gaia_query') | ||
| star_indices, gaia_indices = cross_match_one_to_one({'ra': ra, 'dec': dec}, gaia_data, | ||
| HRDiagram.GAIA_MATCH_RADIUS_ARCSEC) |
There was a problem hiding this comment.
DR3 positions are epoch 2016.0; the test frames are dated 2026-05, so Δt ≈ 10.3 yr. The comment at line 25-27 justifies the radius as tolerating "~a decade of <100 mas/yr proper motion", which puts the tolerance exactly at the failure boundary now, and past it in the future.
The fix should be cheap because pmra/pmdec are already queried: you'd need to propagate to the frame's DATE-OBS before matching (SkyCoord.apply_space_motion). That also lets you tighten the radius to ~0.5", which would cut spurious matches.
There was a problem hiding this comment.
Exactly the kind of comments on correctness I was looking for!
| # histogram the proper motions around their median and take the densest cell as the clump | ||
| bins = np.arange(-PM_HISTOGRAM_HALF_WIDTH, PM_HISTOGRAM_HALF_WIDTH + PM_HISTOGRAM_BIN, PM_HISTOGRAM_BIN) | ||
| median_pmra, median_pmdec = np.median(pmra_finite), np.median(pmdec_finite) | ||
| histogram, pmra_edges, pmdec_edges = np.histogram2d(pmra_finite - median_pmra, pmdec_finite - median_pmdec, bins=(bins, bins)) |
There was a problem hiding this comment.
histogram2d discards out-of-range points, so any cluster more than PM_HISTOGRAM_HALF_WIDTH = 25 mas/yr from the field median is invisible. The docstring at line 159-161 claims robustness "to the cluster being a minority of the field", which is precisely the case that would fail.
To fix this: centre the histogram on the median but widen the range, or better, histogram over the actual data range (bins from np.percentile(pm, [1, 99])) so the window adapts. Also worth returning None when the peak sits against the window edge, which is the signature of a clipped clump.
| else: | ||
| ra, dec = ra_dec_from_wcs(fits_path, cat_data, basename) | ||
|
|
||
| valid = np.isfinite(ra) & np.isfinite(dec) & np.isfinite(mag) & np.isfinite(magerr) |
There was a problem hiding this comment.
This keeps every row with finite values. The CAT table carries flag (SExtractor bits), peak, fwhm and ellipticity, all unused.
Saturated stars have systematically too-faint magnitudes and land at the bright end of the CMD, on top of the turnoff and giant branch. A cluster core will have a far higher deblend fraction than these fields. Filtering for flag == 0 (or at least excluding 4/8/16) and peak < SATURATE is a few lines and improves / cleans the diagram.
sfoale
left a comment
sfoale
left a comment
There was a problem hiding this comment.
Quite a few comments. Addressing some could improve the quality of the results. Overall it looks good though.
…n/max/filters of input fits files.
|
I've tried to address all the comments, especially the ones on algorithm changes for correctness. I've tested the output and everything looks the same with M44 as before, but I'm sure on other clusters things will be much improved with the estimates. |
2 participants
These are the backend changes for the HR Diagram operation. There is not too much happening here so I think it's all pretty straightforward... Maybe just too many constants and defensive programming from the AI overlords
Basically takes in two input images, a "blue" and "red" filter one, of the same target. Also takes in cluster center (ra/dec/name) and search radius. Does the following steps: