DM-42642: Introduce MatrixPlot plot type to analysis_tools #190

enourbakhsh · 2024-01-24T22:21:10Z

No description provided.

taranu

Looks mostly fine; see specific comments.

python/lsst/analysis/tools/actions/plot/matrixPlot.py

taranu · 2024-01-30T19:55:16Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+            The resulting figure.
+        """
+        matrix = data[self.matrixKey]
+        if self.vmin is not None and self.vmax is not None:


I think you should be able to manually override either one of vmin/vmax and still let the other be determined automatically.

Good point! Done!

taranu · 2024-01-30T20:00:16Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+                axis.set_tick_params(which="minor", length=0)
+        else:
+            # Set the desired tick values and labels if provided.
+            if self.xAxisTickValues is not None:


Just noting that If it's left like this instead of if self.xAxisTickValues, then setting self.xAxisTickValues=[] (instead of None) will remove all ticks. I think it's ok to support remove axis ticks and labels but maybe there should be a note about this in the relevant config field docs.

The new approach uses if self.xAxisTickValues.

Hmm, so you don't want users to be able to turn them off entirely (by supplying an empty list)?

Changed it to if self.xAxisTickValues is not None; this allows supplying an empty list.

taranu

A few more notes; I'm assuming the rebase will be trivial.

python/lsst/analysis/tools/actions/plot/matrixPlot.py

taranu · 2024-02-13T18:10:38Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+    )
+
+    xAxisTickLabels = DictField[float, str](
+        doc="Dictionary of x-axis tick positions and labels. These can work independent of "


These are applied as minor ticks, right? Maybe add a longer note about that in the class docstring, especially if it's possible to misconfigure them such that the major and minor tick labels overlap.

They are not restricted to specific tick types. If any keys in this DictField do not align with major ticks provided by xAxisTickValues (or automatically set by matplotlib), they will be set as minor ticks. Thus, these tick labels operate independently, meaning their corresponding positions do not need to match those in xAxisTickValues or anything else. The code automatically adjusts to handle any overlaps caused by user input. I'm not sure if someone can misconfigure this.

Provided additional clarification to the doc.

taranu · 2024-02-13T18:11:00Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+
+    xAxisTickLabels = DictField[float, str](
+        doc="Dictionary of x-axis tick positions and labels. These can work independent of "
+        "`xAxisTickValues`.",


I'm not sure if this reference is going to be parsed in the docs.

I've seen examples in the code base, e.g. here. It doesn't have to be clickable.

taranu · 2024-02-13T18:14:21Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+            default_limits = (None, None)
+
+        # Set the value range using overrides or defaults.
+        vrange = (self.vmin or default_limits[0], self.vmax or default_limits[1])


I think you still need to put default_limits[0] if self.vmin is None else self.vmin in case self.vmin == 0, which evaluates as False (and same for vmax although I don't think vmax=0 would work very well if at all).

Good point, done!

taranu · 2024-02-13T18:16:56Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+                axis.set_tick_params(which="minor", length=0)
+        else:
+            # Set the desired tick values and labels if provided.
+            if self.xAxisTickValues is not None:


Hmm, so you don't want users to be able to turn them off entirely (by supplying an empty list)?

enourbakhsh

Thank you for the review! I'll proceed with the rebase as soon as you're satisfied with the updates.

enourbakhsh · 2024-02-13T20:19:50Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+    )
+
+    xAxisTickLabels = DictField[float, str](
+        doc="Dictionary of x-axis tick positions and labels. These can work independent of "


They are not restricted to specific tick types. If any keys in this DictField do not align with major ticks provided by xAxisTickValues (or automatically set by matplotlib), they will be set as minor ticks. Thus, these tick labels operate independently, meaning their corresponding positions do not need to match those in xAxisTickValues or anything else. The code automatically adjusts to handle any overlaps caused by user input. I'm not sure if someone can misconfigure this.

enourbakhsh · 2024-02-13T20:24:40Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+    )
+
+    xAxisTickLabels = DictField[float, str](
+        doc="Dictionary of x-axis tick positions and labels. These can work independent of "


Provided additional clarification to the doc.

enourbakhsh · 2024-02-13T22:12:22Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+
+    xAxisTickLabels = DictField[float, str](
+        doc="Dictionary of x-axis tick positions and labels. These can work independent of "
+        "`xAxisTickValues`.",


I've seen examples in the code base, e.g. here. It doesn't have to be clickable.

enourbakhsh · 2024-02-13T23:01:46Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+            default_limits = (None, None)
+
+        # Set the value range using overrides or defaults.
+        vrange = (self.vmin or default_limits[0], self.vmax or default_limits[1])


Good point, done!

enourbakhsh · 2024-02-13T23:19:48Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+                axis.set_tick_params(which="minor", length=0)
+        else:
+            # Set the desired tick values and labels if provided.
+            if self.xAxisTickValues is not None:


Changed it to if self.xAxisTickValues is not None; this allows supplying an empty list.

taranu · 2024-02-21T00:43:58Z

python/lsst/analysis/tools/actions/plot/matrixPlot.py

+
+        # Set the colorbar and draw the image.
+        norm = ImageNormalize(vmin=vrange[0], vmax=vrange[1])
+        img = ax.imshow(matrix, interpolation="none", norm=norm)


I think the origin kwarg should be configurable here (I prefer lower which is unambiguously the correct choice for our actual CCD images but making it a ChoiceField[str] with upper as the default is fine if that's the convention for covariance plots for the camera team).

Done! I have made it configurable so that we can revisit this decision in the future.

enourbakhsh force-pushed the tickets/DM-42642 branch 6 times, most recently from 4832c1a to c44ea87 Compare January 30, 2024 01:37

enourbakhsh requested a review from taranu January 30, 2024 19:29

taranu reviewed Jan 30, 2024

View reviewed changes

enourbakhsh force-pushed the tickets/DM-42642 branch from c44ea87 to bf76b18 Compare January 31, 2024 16:44

enourbakhsh force-pushed the tickets/DM-42642 branch 2 times, most recently from 76075d6 to d5ce4f6 Compare February 7, 2024 23:05

enourbakhsh changed the title ~~DM-42642: Make the MatrixPlot plot type for analysis_tools~~ DM-42642: Introduce MatrixPlot plot type to analysis_tools Feb 8, 2024

taranu reviewed Feb 13, 2024

View reviewed changes

enourbakhsh commented Feb 13, 2024

View reviewed changes

enourbakhsh force-pushed the tickets/DM-42642 branch 2 times, most recently from 71e4859 to 537b17e Compare February 21, 2024 00:34

taranu reviewed Feb 21, 2024

View reviewed changes

enourbakhsh force-pushed the tickets/DM-42642 branch from c3b3ff6 to e31c9a7 Compare February 21, 2024 04:24

Add MatrixPlot plot type

b61e544

enourbakhsh force-pushed the tickets/DM-42642 branch from e31c9a7 to b61e544 Compare February 21, 2024 20:32

enourbakhsh merged commit 01c83d5 into main Feb 22, 2024
8 checks passed

enourbakhsh deleted the tickets/DM-42642 branch February 22, 2024 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-42642: Introduce MatrixPlot plot type to analysis_tools #190

DM-42642: Introduce MatrixPlot plot type to analysis_tools #190

enourbakhsh commented Jan 24, 2024

taranu left a comment

taranu Jan 30, 2024

enourbakhsh Feb 6, 2024

taranu Jan 30, 2024

enourbakhsh Feb 7, 2024

taranu Feb 13, 2024

enourbakhsh Feb 13, 2024

taranu left a comment

taranu Feb 13, 2024

enourbakhsh Feb 13, 2024

enourbakhsh Feb 13, 2024

taranu Feb 13, 2024

enourbakhsh Feb 13, 2024 •

edited

taranu Feb 13, 2024

enourbakhsh Feb 13, 2024

taranu Feb 13, 2024

enourbakhsh left a comment

enourbakhsh Feb 13, 2024

enourbakhsh Feb 13, 2024

enourbakhsh Feb 13, 2024 •

edited

enourbakhsh Feb 13, 2024

enourbakhsh Feb 13, 2024

taranu Feb 21, 2024

enourbakhsh Feb 21, 2024

DM-42642: Introduce MatrixPlot plot type to analysis_tools #190

DM-42642: Introduce MatrixPlot plot type to analysis_tools #190

Conversation

enourbakhsh commented Jan 24, 2024

taranu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taranu 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

Choose a reason for hiding this comment

enourbakhsh Feb 13, 2024 • edited

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

enourbakhsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enourbakhsh Feb 13, 2024 • edited

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

enourbakhsh Feb 13, 2024 •

edited

enourbakhsh Feb 13, 2024 •

edited