search

icecube / skyreader

An API for Results Produced by SkyDriver & the Skymap Scanner
MIT License
0 stars 0 forks source link

Plotting functions cleanup #21

Closed mlincett closed 1 year ago

mlincett commented 1 year ago

This PR cleans up a bit the plotting code, but does not yet address #22.

Important: .npz files were removed from skymap_scanner, hence the CI testing for npz reading was failing. I have removed such test altogether as I believe we are not going to support .npz anymore in the future.

mlincett commented 1 year ago

Looks good! I added a few comments.

I was the one who removed the npz test files from skymap-scanner, as I didn't see it being used in any of its tests. I didn't realize they were still used here but it's ok as long as we keep npz support for a bit longer, i have some older scans that only exists in that format.

I added the npz test exactly for the purpose of the CI testing of the Skymap Scanner (we did not want the CI testing of the scanner failing because of SkyReader not reading npz), so no big deal indeed.

mlincett commented 1 year ago

I have added some minor modifications for partial compliance with mypy and put remaining errors as comment.

Some of mypy complaints appear like they should raise runtime errors, but they don't. Maybe it's just matplotlib being still a bit of a mess, but it's good to keep an eye on those when we refactor the code.

ric-evans commented 1 year ago

Looks good! I added a few comments. I was the one who removed the npz test files from skymap-scanner, as I didn't see it being used in any of its tests. I didn't realize they were still used here but it's ok as long as we keep npz support for a bit longer, i have some older scans that only exists in that format.

I added the npz test exactly for the purpose of the CI testing of the Skymap Scanner (we did not want the CI testing of the scanner failing because of SkyReader not reading npz), so no big deal indeed.

@mlincett @tianluyuan how about we add a separate script to convert npz to json? that way, we can remove npz support without abandoning the old format entirely

tianluyuan commented 1 year ago

Looks good! I added a few comments. I was the one who removed the npz test files from skymap-scanner, as I didn't see it being used in any of its tests. I didn't realize they were still used here but it's ok as long as we keep npz support for a bit longer, i have some older scans that only exists in that format.

I added the npz test exactly for the purpose of the CI testing of the Skymap Scanner (we did not want the CI testing of the scanner failing because of SkyReader not reading npz), so no big deal indeed.

@mlincett @tianluyuan how about we add a separate script to convert npz to json? that way, we can remove npz support without abandoning the old format entirely

yea that could work as a fail-safe for older scans and I would be fine with removing the npz support once the script is available.