New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-29421: Add AP plots in analysis_ap using analysis_tools #32
Conversation
a96d112
to
bb66765
Compare
90d626b
to
ef0dd6e
Compare
As part of the review, I need some assistance getting the modified pipeline to run before this is ready to merge. At the moment, when I run
The result is as follows
|
ee2cc67
to
1234289
Compare
This should be all set now and ready for your actual review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to consider
@@ -20,13 +20,16 @@ | |||
# along with this program. If not, see <https://www.gnu.org/licenses/>. | |||
from __future__ import annotations | |||
|
|||
__all__ = "PlotConfig" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be in () like __all__ = ("PlotConfig", )
otherwise some things will iterate over the string
def setDefaults(self): | ||
super().setDefaults() | ||
|
||
self.process.buildActions.ras = LoadVector() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need process at all here you can just do:
self.prep.vectorKeys = ['ra', 'decl']
...
self.produce.panels['panel_main'].ra = 'ra'
self.produce.panels['panel_main'].dec = 'decl'
Again, this is just an alternative way to write this that may be more convenient, or may not be depending on how you think about things
|
||
|
||
class PanelConfig(Config): | ||
"""Configuration options for plot panels.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a bit more of a doc string, its not really panels for ANY plot
|
||
class DiaSkyPlot(PlotAction): | ||
"""Generic pseudo base class for plotting DiaSources | ||
(or DiaObjects) on the sky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc string seems awkward for some reason, is the line break in the right place?
1234289
to
7b3d4f3
Compare
Also add a PanelConfig class to plotUtils that can be used to turn various matplotlib knobs pertaining to subplot layout.
7b3d4f3
to
4503563
Compare
This PR adds a new plotting capability for DiaSources on the sky. The simple version of this plot is here in
analysis_tools
, and a specialized version for a specific AP QA data set is inanalysis_ap
.