725 integration tests of metrics #750
Conversation
… catalog. - rewrite test_metrics to use class-scoped fixtures for processed data - add constant to avoid 0 weight in metrics.py::match_by_locs - add functions to move FullCatalog to device
…/bliss into 725-integration-tests-of-metrics
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
I pulled the changes to metrics from 288718d, so most of the changes in this PR are from there. I didn't realize they would also show up on the diffs here - let me know if there's a better way to handle that, like closing this and making a new PR after that one's been merged maybe? |
Codecov Report
@@ Coverage Diff @@
## master #750 +/- ##
==========================================
+ Coverage 79.48% 88.33% +8.85%
==========================================
Files 13 13
Lines 1121 1132 +11
==========================================
+ Hits 891 1000 +109
+ Misses 230 132 -98
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…ation-tests-of-metrics
conf/base_config.yaml
Outdated
| min_sources: 0 | ||
| prob_galaxy: 0.72 | ||
| star_flux_min: 622.0 | ||
| star_flux_min: 1e5 |
There was a problem hiding this comment.
622 is more realistic. Can you override this constant in a case study-specific config file rather than changing it throughout?
There was a problem hiding this comment.
Changed this accidentally, reverted to previous value.
conf/base_config.yaml
Outdated
| @@ -32,10 +33,10 @@ simulator: | |||
| n_bands: 1 | |||
| batch_size: 64 | |||
| max_sources: 1 | |||
| mean_sources: 0.2 | |||
| mean_sources: 0.03 | |||
There was a problem hiding this comment.
Better to modify not to change this project-wide. Can you override this constant in a case study-specific config file rather than changing it throughout?
tests/test_catalogs.py
Outdated
| sample_file = Path(cfg.paths.decals).joinpath("tractor-3366m010.fits") | ||
| decals_cat = DecalsFullCatalog.from_file(sample_file) | ||
|
|
||
| assert not torch.all(decals_cat.plocs) # all plocs should be 0 by default |
There was a problem hiding this comment.
Is plocs a floating point value? If so, it's probably better not to check for exact equality
There was a problem hiding this comment.
Do we expect this assertion might fail in the future?
There was a problem hiding this comment.
plocs is manually set to torch.zeros in DecalsFullCatalog.from_file, so the assertion itself shouldn't fail. But there isn't really a need to test this, so I removed the check.
|
I think the metrics bug is actually more complicated than just adding a null check. We need to check all the combinations of the tensors being empty and update the false positives / false negatives accordingly, so I created #752 to handle it separately. |
Added tests for comparing BLISS catalog to DECaLS and SDSS catalogs. Fixes #725.
tests/test_metrics.pyand restructured as class. Data loading is done as class-level fixture to avoid repeated processing.DecalsFullCatalog.from_fileto take optional RA and DEC limits instead of required pixel coord limits and WCS.get_plocs_from_ra_dechandles the conversion to pixel coordinates. Added tests for this functionality intests/test_catalogs.py.to_dict()andto()methods toFullCatalogto move entire catalog to device, similar toTileFullCatalog.