icecube / skyreader

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

Decouple plotting routines from `SkyScanResult` #22

Closed mlincett closed 11 months ago

mlincett commented 11 months ago

The plotting methods of SkyScanResult are fairly complicated and could benefit from some refactoring.

I think it would be sensible to decouple all the plotting stuff from SkyScanResult and collect those in a separate SkyScanPlotter class.

Let me know if you have anything against it @ric-evans and @tianluyuan

I plan to move forward with this in #21

tianluyuan commented 11 months ago

Sounds good. I support this 👍

ric-evans commented 11 months ago

Good idea, we could also use some unit tests to make sure we don't delete something in the refactor

FWIW I run pylint locally which produces an error when files get beyond 1000 lines. some of pylint's errors are more like suggestions than steadfast rules, but I've found a majority of them helpful

ric-evans commented 11 months ago

I wrote a gist for this exact purpose https://gist.github.com/ric-evans/aa55b6d054ef1fae9464b992631aa90d It does require multiple commits to main, but that's unavoidable

Note: You'll have to manually stop the CI to avoid an auto-release

mlincett commented 11 months ago

I wrote a gist for this exact purpose https://gist.github.com/ric-evans/aa55b6d054ef1fae9464b992631aa90d It does require multiple commits to main, but that's unavoidable

Note: You'll have to manually stop the CI to avoid an auto-release

@ric-evans would you mind doing this for me to duplicate results.py?

ric-evans commented 11 months ago

I wrote a gist for this exact purpose gist.github.com/ric-evans/aa55b6d054ef1fae9464b992631aa90d It does require multiple commits to main, but that's unavoidable Note: You'll have to manually stop the CI to avoid an auto-release

@ric-evans would you mind doing this for me to duplicate results.py?

Sure thing -- I'll leave the actual trimming to you. I'll put it on my list for tomorrow

ric-evans commented 11 months ago

@mlincett done. Check out https://github.com/icecube/skyreader/blob/main/skyreader/plot/plot.py, it has duplicate line history (blame) as result.py. You'll now need to remove what you don't want to keep from each file and fix the imports

mlincett commented 11 months ago

Fixed in #28