Some plotting and representation improvements #260

teutoburg · 2023-08-04T15:32:30Z

Some improvements to plotting methods of SpectralTrace, SpectralTraceList and DetectorList. They now use matplotlib's "OOP mode" (i.e. fig, ax instead of all plt.whatever), as a first step to migrate all plotting related functions to that. This allows e.g. to easily plot the spectral traces into the detector layout plot by passing the axes object (this use-case was the main motivation for this modification). More configurability for the plot methods was added along the way, but the default behavior should remain the same as before.
A _repr_pretty_ method was added to some classes, which is accessed by IPython to support a nicer human-readable representation of an object. This is currently very experimental, as there are multiple (not really compatible) ways to implement this. This will require more though, experimentation, and finally standardization. And probably a separate issue...

codecov · 2023-08-04T18:39:42Z

Codecov Report

Patch coverage: 28.24% and project coverage change: -0.30% ⚠️

Comparison is base (c700bc4) 80.85% compared to head (331e12a) 80.55%.

Additional details and impacted files

@@              Coverage Diff               @@
##           dev_master     #260      +/-   ##
==============================================
- Coverage       80.85%   80.55%   -0.30%     
==============================================
  Files             143      143              
  Lines           14676    14736      +60     
==============================================
+ Hits            11866    11871       +5     
- Misses           2810     2865      +55

Files Changed	Coverage Δ
scopesim/optics/optical_train.py	`43.84% <3.70%> (-4.36%)`	⬇️
scopesim/effects/spectral_trace_list_utils.py	`59.56% <11.11%> (-3.28%)`	⬇️
scopesim/optics/optical_element.py	`61.78% <25.00%> (-1.24%)`	⬇️
scopesim/detector/detector_array.py	`86.00% <33.33%> (-11.57%)`	⬇️
scopesim/effects/spectral_trace_list.py	`53.61% <52.94%> (+0.91%)`	⬆️
scopesim/effects/effects.py	`90.58% <70.00%> (+2.08%)`	⬆️
scopesim/effects/detector_list.py	`92.70% <100.00%> (-0.06%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

oczoske · 2023-08-07T15:00:26Z

scopesim/effects/spectral_trace_list.py

-        if len(Gcf.figs) == 0:
-            plt.figure(figsize=(12, 12))
+        if axes is None:
+            fig, axes = plt.subplots(figsize=(12, 12))


Should we keep the absolute figsize or should we leave that to the user? While 12in x 12in makes a nice LP cover, it is a tad too large for my computer screens.

I don't know what the original motivation was for this giant plot, but I thought there might have been a good reason, so I left it in. That said, working on the .plot() methods here motivated me to go over those in general for other classes (PR coming soon!) and the figure creation will be "outsourced" to a new function utils.figure_factory anyway, so the explicit figsize here will most likely go away as a result of that...

Giant plots are needed to see where the spectral traces are actually positioned on the detector plane. However this should only be a default if someone hasn't already generated a figure or passed an Axes object.
Feel free to change this to something else if it's bothersome 👍

scopesim/effects/spectral_trace_list_utils.py

astronomyk · 2023-08-09T10:22:13Z

scopesim/effects/spectral_trace_list.py

            hdu = self[trace_id].rectify(hdulist,
                                         interps=interps,
                                         bin_width=bin_width,
                                         xi_min=xi_min, xi_max=xi_max,
                                         wave_min=wave_min, wave_max=wave_max)
            if hdu is not None:   # ..todo: rectify does not do that yet
                outhdul.append(hdu)
-                outhdul[0].header[f"EXTNAME{i+1}"] = trace_id
+                outhdul[0].header[f"EXTNAME{i}"] = trace_id


i is in the range [0, len(spec traces)] from the enumerate call in line 302
The primary header EXTNAME0 will (should) never contain trace data.
Trace data should only be held in the additional FITS extensions 1..N, i.e. EXTNAME[1 ... len(spec traces)]

If you look a few lines further up, I also changed the enumerate to enumerate(self.spectral_traces, start=1), which means i is in range(1, len(self.spectral_traces) + 1). This (in general, not only in this example) has the benefits of more clearly marking the purpose of i (we want values from 1, not 0) and saving to type +1 every time (which may cause bugs if forgotten once). Of course, this only makes sense if i is only used as i+1 within that particular scope (which is the case here).

Today I learned ... :)

astronomyk · 2023-08-09T10:29:23Z

scopesim/effects/spectral_trace_list_utils.py

        aperture_id = self.meta["aperture_id"]
        lam_arr = self.table[self.meta["wave_colname"]]

        wave_max = np.max(lam_arr)
        wave_min = np.min(lam_arr)

        return {"wave_min": wave_min, "wave_max": wave_max,
-                "trace_id": trace_id, "aperture_id": aperture_id}
+                "trace_id": self.trace_id, "aperture_id": aperture_id}


Where is self.trace_id first defined? I can't find it in __init__

Or is this one of those new-fangled things that _class_param dict entries can be accessed through self.?

It's a property that already existed anyway and is defined in (new) line 599. It should probably be a bit further up, it's somewhat "hidden in the jungle" now. I wrote that in my general list and will include it later.

astronomyk

Apart from the question I have regarding the use of self.trace_id when this is not explicitly available from self.or in the_class_paramsdict, and the change back toEXTNAME{i+1}`, it looks good to go.

Good job!

astronomyk · 2023-08-09T10:30:51Z

scopesim/effects/spectral_trace_list_utils.py

        aperture_id = self.meta["aperture_id"]
        lam_arr = self.table[self.meta["wave_colname"]]

        wave_max = np.max(lam_arr)
        wave_min = np.min(lam_arr)

        return {"wave_min": wave_min, "wave_max": wave_max,
-                "trace_id": trace_id, "aperture_id": aperture_id}
+                "trace_id": self.trace_id, "aperture_id": aperture_id}


Or is this one of those new-fangled things that _class_param dict entries can be accessed through self.?

astronomyk · 2023-08-09T10:32:01Z

scopesim/effects/spectral_trace_list_utils.py

@@ -207,7 +206,7 @@ def map_spectra_to_focal_plane(self, fov):
            xilam = XiLamImage(fov, self.dlam_per_pix)
            self._xilamimg = xilam   # ..todo: remove or make available with a debug flag?
        except ValueError:
-            print(f" ---> {self.meta['trace_id']} gave ValueError")
+            print(f" ---> {self.trace_id} gave ValueError")


Same as comment above

astronomyk · 2023-08-09T10:34:40Z

scopesim/effects/spectral_trace_list_utils.py

@@ -548,7 +624,7 @@ def __repr__(self):
        return f"{self.__class__.__name__}({self.table!r}, **{self.meta!r})"

    def __str__(self):
-        msg = (f"<SpectralTrace> \"{self.meta['trace_id']}\" : "
+        msg = (f"<SpectralTrace> \"{self.trace_id}\" : "


Same as above with self.trace_id

teutoburg added 3 commits August 4, 2023 17:15

Improved plot methods using mpl OOP API and more configs

2f39b67

Define some _repr_pretty_ methods for IPython display

50f92de

Fix failing report because of fig/ax change

92de6b4

teutoburg added 2 commits August 5, 2023 00:16

Minor improvements along the way...

f416ded

Mostly docstrings

fd72a2e

teutoburg marked this pull request as ready for review August 5, 2023 00:52

teutoburg requested review from astronomyk and oczoske August 5, 2023 00:53

oczoske reviewed Aug 7, 2023

View reviewed changes

scopesim/effects/spectral_trace_list_utils.py Outdated Show resolved Hide resolved

oczoske approved these changes Aug 7, 2023

View reviewed changes

oczosked

331e12a

astronomyk reviewed Aug 9, 2023

View reviewed changes

astronomyk requested changes Aug 9, 2023

View reviewed changes

teutoburg merged commit 28dc5dc into AstarVienna:dev_master Aug 9, 2023
17 of 18 checks passed

teutoburg deleted the fh/plots-and-reprs branch August 9, 2023 22:44

teutoburg mentioned this pull request Aug 23, 2023

Figure factory #263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some plotting and representation improvements #260

Some plotting and representation improvements #260

teutoburg commented Aug 4, 2023

codecov bot commented Aug 4, 2023 •

edited

oczoske Aug 7, 2023

teutoburg Aug 7, 2023

astronomyk Aug 9, 2023

astronomyk Aug 9, 2023

teutoburg Aug 9, 2023

astronomyk Aug 10, 2023

astronomyk Aug 9, 2023

astronomyk Aug 9, 2023

teutoburg Aug 9, 2023

astronomyk left a comment

astronomyk Aug 9, 2023

astronomyk Aug 9, 2023

astronomyk Aug 9, 2023

Some plotting and representation improvements #260

Some plotting and representation improvements #260

Conversation

teutoburg commented Aug 4, 2023

codecov bot commented Aug 4, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astronomyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 4, 2023 •

edited