Skip to content
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-43338: Add a debugPSF pipeline and associated plots #276

Merged
merged 5 commits into from
Aug 13, 2024
Merged

Conversation

sr525
Copy link
Collaborator

@sr525 sr525 commented Jul 16, 2024

No description provided.

@sr525 sr525 force-pushed the tickets/DM-43338 branch 3 times, most recently from 7a01a1a to ba8d068 Compare July 22, 2024 16:37
Copy link

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Sophie,
Thanks for getting this started! The overall design looks great, but I think there are some implementation bugs. See also my comments on the plots on jira.

config:
connections.outputName: debugCoaddPsf
# set plots to run
atools.ap12PsfSky: Ap12PsfSkyPlot # photometry.py

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[comments] too few spaces before comment

Should probably make the linter happy if it's going to be running all the time...

config:
connections.outputName: debugVisitPsf
atools.e1DiffScatter: E1DiffScatterVisit # shapes.py
atools.e2Diffscatter: E2DiffScatterVisit # shapes.py

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
atools.e2Diffscatter: E2DiffScatterVisit # shapes.py
atools.e2DiffScatter: E2DiffScatterVisit # shapes.py

self.process.calculateActions.stars.lowSNSelector.fluxType = "psfFlux"
self.process.calculateActions.stars.fluxType = "psfFlux"

self.process.filterActions.yStars = LoadVector(vectorKey="e1Diff")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Can we use something other than e1Diff as a place-holder that must be overridden by subclasses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I think this might be the same problem that made the plots look the same.

"""

def setDefaults(self):
super().setDefaults()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
super().setDefaults()
super().setDefaults()
self.process.filterActions.yStars = LoadVector(vectorKey="e2Diff")

I think you need something like this?

self.process.filterActions.yStars = DownselectVector(
vectorKey="e1Diff", selector=VectorSelector(vectorKey="starSelector")
)
self.applyContext(VisitContext)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.applyContext(VisitContext)
self.applyContext(VisitContext)
self.process.filterActions.yStars = LoadVector(vectorKey="fracDiff")

Probably need this?

super().setDefaults()
self.applyContext(CoaddContext)
self.process.filterActions.yStars = LoadVector(vectorKey="e1Diff")
self.produce.plot.yAxisLabel = "Ellipticty residuals (e1 - e1_PSF)"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.produce.plot.yAxisLabel = "Ellipticty residuals (e1 - e1_PSF)"
self.produce.plot.yAxisLabel = "Ellipticity residuals (e1 - e1_PSF)"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And elsewhere...

self.produce.plot.yAxisLabel = "Ellipticty residuals (e2 - e2_PSF)"
self.produce.plot.zAxisLabel = "E2 Diff"
self.produce.plot.plotName = "E2 Diff Sky"
self.applyContext(CoaddContext)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need both contexts here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we do not.

@@ -222,3 +506,271 @@ def setDefaults(self):
self.process.buildActions.statMask = SnSelector()
self.process.buildActions.statMask.threshold = 20
self.process.buildActions.statMask.fluxType = "psfFlux"


class BaseElips(AnalysisTool):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere? Can we delete this class? (and associated entry in __all__)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.

@sr525 sr525 force-pushed the tickets/DM-43338 branch 5 times, most recently from 6e912e4 to ef465f0 Compare August 2, 2024 04:08
class E1FocalPlane(EFocalPlane):
"""A focal plane plot of E1."""

parameterizedBand: bool = False

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be factored out to the base class? (And similar in E?SkyVisit?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe all the way to EBase even...

Copy link

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks Sophie!

@sr525 sr525 merged commit 2b45b7d into main Aug 13, 2024
8 checks passed
@sr525 sr525 deleted the tickets/DM-43338 branch August 13, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants