Fix flux unit #1060
Fix flux unit #1060
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1060 +/- ##
==========================================
+ Coverage 80.73% 81.21% +0.47%
==========================================
Files 23 23
Lines 3073 3045 -28
==========================================
- Hits 2481 2473 -8
+ Misses 592 572 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bliss/encoder/metrics.py
Outdated
|
|
||
| def get_state_for_report(self, state_name): | ||
| state = getattr(self, state_name) | ||
| match self.report_bin_unit: | ||
| case "nmgy": | ||
| case "njy": |
There was a problem hiding this comment.
I don't use njy for SDSS, only nmgy
There was a problem hiding this comment.
I wish there was a generic term that could mean either
There was a problem hiding this comment.
change it to flux
bliss/catalog.py
Outdated
| """ | ||
|
|
||
| return 22.5 - 2.5 * torch.log10(nmgy / 3631) | ||
| return 22.5 - 2.5 * torch.log10(flux / c) |
There was a problem hiding this comment.
Better to subtract log10(c) for stability
There was a problem hiding this comment.
Presumably the 22.5 can be absorbed by the log10(c) term
There was a problem hiding this comment.
Perhaps call c the zero_point. It's more descriptive
There was a problem hiding this comment.
remove the 22.5 and rename c to zero_point
bliss/catalog.py
Outdated
| def on_njymag(self) -> Tensor: | ||
| return convert_nmgy_to_njymag(self.on_nmgy) | ||
| def on_magnitudes(self, c) -> Tensor: | ||
| return convert_flux_to_magnitude(self.on_fluxes, c) |
There was a problem hiding this comment.
Where is c defined? I believe c is survey specific.
There was a problem hiding this comment.
so we could store these in the Survey subclasses.
There was a problem hiding this comment.
I tried to modify the Survey class but find that we also need to change the Encoder and some notebooks. So I move the zero point specification to our metrics by adding one argument mag_zero_point to our metric classes.
bliss/conf/base_config.yaml
Outdated
| @@ -143,17 +143,17 @@ variational_factors: | |||
| metrics: | |||
| detection_performance: | |||
| _target_: bliss.encoder.metrics.DetectionPerformance | |||
| base_nmgy_bin_cutoffs: [200, 400, 600, 800, 1000] | |||
| report_bin_unit: njymag | |||
| base_njy_bin_cutoffs: [200, 400, 600, 800, 1000] | |||
There was a problem hiding this comment.
For SDSS, it wouldn't make sense to specify anything in terms of nJy. Magnitude is something of a universal system of flux: perhaps we could always specify the cutoffs in terms of magnitude?
There was a problem hiding this comment.
Alternatively, we could call these flux_bin_cutoffs, and use the default units for the survey (without specifying what they are). I'm not sure which way is better.
There was a problem hiding this comment.
change it to base_flux_bin_cutoffs
bliss/encoder/metrics.py
Outdated
| report_bins = torch.flip(report_bins, dims=(-1,)) | ||
| case "njymag": | ||
| report_bins = convert_nmgy_to_njymag(report_bins) | ||
| case "ab_mag": |
There was a problem hiding this comment.
Maybe call this "mag" rather than "ab_mag"? Apparently SDSS magnitudes aren't exactly ab magnitudes
There was a problem hiding this comment.
change it to mag
catalog.pyis problematic; fix it in this PRto_tile_catalogto_tile_cataloghave invalid indices