Plotting functions cleanup #21

mlincett · 2023-09-29T13:21:41Z

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.

ric-evans · 2023-10-09T20:22:23Z

skyreader/result.py


        # Output contours in RA, dec instead of theta, phi
-        saving_contours = []
+        saving_contours: list = []


This isn't needed unless you subscript a sub-type

I think mypy was complaining here.

skyreader/result.py:1097: error: Need type annotation for "saving_contours" (hint: "saving_contours: List[<type>] = ...") [var-annotated]

odd... but okay if mypy says so!

skyreader/result.py

ric-evans · 2023-10-09T20:32:26Z

skyreader/result.py

-                extra_header = fits_header, overwrite=True)
+        # Dump the whole contour
+        path = unique_id + ".contour.pkl"
+        print("Saving contour to", path)


You have several prints. Generally, I recommend avoiding this pattern for anything other than scripts (I see this package as a library) and using logs instead. What's the intended use of these plotting functions?

Yes, I don't like it either, it is a pattern inherited from the old code. Surely we can implement proper loggers.

I would say this is a todo for a future PR.

sure, can you make an issue for it?

ric-evans

A few comments for you.

The rest looks more up @tianluyuan's wheelhouse

tianluyuan

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.

skyreader/result.py

mlincett · 2023-10-10T16:24:58Z

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 · 2023-10-11T08:30:47Z

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 · 2023-10-11T19:41:26Z

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 · 2023-10-11T21:45:23Z

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.

mlincett and others added 7 commits September 29, 2023 14:49

deprecate log_func

92cfc29

<bot> update requirements-examples.txt

01931ce

<bot> update requirements-tests.txt

bfdf70e

<bot> update requirements.txt

036a620

deprecate upload-func

478b2af

deprecate further slack posting logic

36b94d3

further remove deprecated logic

ff212e4

mlincett marked this pull request as draft September 29, 2023 13:21

assume we always save plots

5d23c1b

mlincett mentioned this pull request Sep 29, 2023

Decouple plotting routines from SkyScanResult #22

Closed

mlincett and others added 12 commits September 29, 2023 15:53

single source of truth for inches/dpi

e8cee41

remove unused vars

6b6840d

remove import io

d3f4431

update example

1ee865a

de-type plotting function

cda9860

some notes from mypy

3d6747f

mypy readiness step

b0730dc

deprecate test for npz format

8270f73

<bot> update setup.cfg

7b9e266

<bot> update requirements-examples.txt

d40342a

<bot> update requirements-tests.txt

8d1ee52

<bot> update requirements.txt

fbfe60c

mlincett marked this pull request as ready for review October 9, 2023 14:22

mlincett changed the title ~~Plotting functions updates~~ Plotting functions cleanup Oct 9, 2023

mlincett requested review from ric-evans and tianluyuan October 9, 2023 15:58

ric-evans reviewed Oct 9, 2023

View reviewed changes

skyreader/result.py Outdated Show resolved Hide resolved

ric-evans reviewed Oct 9, 2023

View reviewed changes

mlincett added 2 commits October 10, 2023 11:12

use new interface to colormaps

a9c454f

list to array

7a0d644

tianluyuan reviewed Oct 10, 2023

View reviewed changes

skyreader/result.py Outdated Show resolved Hide resolved

skyreader/result.py Show resolved Hide resolved

skyreader/result.py Show resolved Hide resolved

mlincett added 4 commits October 11, 2023 09:44

calculate area

a08488d

make it a static metod

86e2d46

annotate mypy errors

bcf288e

partial mypy compliance; annotate errors

57b5231

mlincett requested review from ric-evans and tianluyuan October 11, 2023 12:40

ric-evans approved these changes Oct 11, 2023

View reviewed changes

tianluyuan approved these changes Oct 11, 2023

View reviewed changes

mlincett merged commit 97dff8e into main Oct 12, 2023

ric-evans deleted the plotting-updates branch October 13, 2023 15:49

Plotting functions cleanup #21

Plotting functions cleanup #21

Uh oh!

Conversation

mlincett commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ric-evans left a comment

Choose a reason for hiding this comment

Uh oh!

tianluyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mlincett commented Oct 10, 2023

Uh oh!

mlincett commented Oct 11, 2023

Uh oh!

ric-evans commented Oct 11, 2023

Uh oh!

tianluyuan commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mlincett commented Sep 29, 2023 •

edited

Loading