To tile coords

[XinyueLi1012]

avoid for loop over tiles
-- use sort(), it would be slow when the image is big (eg. 4000* 4000)

[codecov]

Codecov Report

Merging #939 (d4e91c9) into master (0a8b6db) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
+ Coverage   95.82%   95.88%   +0.05%     
==========================================
  Files          26       26              
  Lines        3211     3233      +22     
==========================================
+ Hits         3077     3100      +23     
+ Misses        134      133       -1     

Flag	Coverage Δ
unittests	95.88% <100.00%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
bliss/catalog.py	99.66% <100.00%> (+0.39%)	⬆️
bliss/data_augmentation.py	98.43% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jeff-regier]

Great work @XinyueLi1012! I timed both the old and new versions of to_tile_coords: your new one is more than 10 times faster (0.11 seconds vs 1.6 seconds).

The output from both implementations usually matches, so I think the new implementation is at least nearly right. (Possibly it is perfect and the old implementation is slightly off.) But I'm reluctant to merge until they match exactly or until we're really confident that the new version is correct. Please see my trial below. The difference may have to do with how these two implementations handle negative locations. Should we be filtering sources with out-of-bounds locations (either too high or too low) as part of our data augmentation procedure?

> new_tp = aug_full.to_tile_params(4, 4)
> old_tp = aug_full.old_to_tile_params(4, 4)
> aug_full.n_sources.sum()
tensor(4681, device='cuda:0')
> new_tp.n_sources.sum()
tensor(4681, device='cuda:0')
> old_tp.n_sources.sum()
tensor(4681, device='cuda:0')
> (new_tp.locs < 0).sum()
tensor(0, device='cuda:0')
> (old_tp.locs < 0).sum()
tensor(190, device='cuda:0')
> new_tp.locs.isclose(old_tp.locs).sum()
tensor(204610, device='cuda:0')
> (~new_tp.locs.isclose(old_tp.locs)).sum()
tensor(190, device='cuda:0')

[jeff-regier]

Also, would you add a test to verify that if we rotate a catalog by 90 degrees and next by 270 degree, then we get back the original catalog? (Or do we already have a test like that?)

[XinyueLi1012]

Thanks for the testing results. I didn't do much testing for the new implementation so I'm also a little worry about some special cases. I'll add some tests for the new implementation and data augmentation.

[XinyueLi1012]

@jeff-regier I added some filtering for the negative "locs" in the to_tile_params (easier than filter in data augmentation)

[jeff-regier]

Thanks @XinyueLi1012. Before we merge, can you also verify that our performance on DC2 stays the same with this PR?

[jeff-regier]

This is a good test to have, but I realize it doesn't quite test what I thought it would, because there's no conversion btw tile and full catalogs happening.

Would you add an additional test that converts from a tile catalog to a full catalog and then back again to a tile catalog? (And that the first and last tile catalogs are equal?)

[XinyueLi1012]

Yes make sense

[jeff-regier]

I think it'd be better to do the filtering only on demand: better to give users an error if they try to convert catalog formats without first removing out-of-bounds locations. Would you a new method called something like filter_out_of_bounds? And then call that method following data augmentation?

[XinyueLi1012]

That would be tricky. Let me have a try first.

[jeff-regier]

Here's an alternative that may be less tricky: add a keyword argument to to_tile_coords called filter_oob which is false by default. Only if it's true will we filter light sources that are out of bounds (either too negative or too large).

[XinyueLi1012]

that's clever

[jeff-regier]

Please shorten a variable name here or somehow rewrite this assignment so there isn't a stray -1 alone on a line.

[XinyueLi1012]

Thanks @XinyueLi1012. Before we merge, can you also verify that our performance on DC2 stays the same with this PR?

Some times when I run new to_tile_coords on full dc2 catalog (image size 4000*4000) in a jupyter notebook, the kernel dead, I'm digging into it to find if there is any bug. The performance using new to_tile_coords should be similar with the results we got previously.