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-42043: add deltaSkyCorr plot to analysis_tools #174

Merged
merged 1 commit into from Jan 3, 2024

Conversation

aemerywatkins
Copy link

No description provided.


def __call__(self, data: KeyedData, **kwargs) -> Scalar:
if len(data[self.histKey.format(**kwargs)]) != 0:
hist = cast(Vector, data[self.histKey.format(**kwargs)])
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't really need any casts here, unless you are thinking histMedian should be typed with Vector Scalar etc.

@aemerywatkins aemerywatkins force-pushed the tickets/DM-42043 branch 2 times, most recently from c6cb005 to 9be3759 Compare December 14, 2023 19:40
@@ -246,3 +246,90 @@ def __call__(self, data: KeyedData, **kwargs) -> Scalar:
mask = self.getMask(**kwargs)
arr = cast(Vector, data[self.vectorKey.format(**kwargs)])[mask]
return cast(Scalar, np.nansum(arr))


class MedianHistAction(ScalarAction):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need to add MedianHistAction and IqrHistAction to the __all__.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done by @aemerywatkins.

return med


class IqrHistAction(ScalarAction):
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is for IQRHistAction, but I admit that this style is more in keeping with other aspects of the code base in this file (e.g., sigmaMad). I will leave it up to you as to your preference (also a question for @sr525).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also elsewhere where we use PsfFlux (for example) rather than PSFFlux. So my vote is for IqrHistAction.

Comment on lines 296 to 297
histKey = Field[str]("Key of frequency Vector to median")
midKey = Field[str]("Key of bin midpoints Vector to median")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the to median parts of the sentences here (and in the other class as well) - I think it reads better that way.

-------
delta_skyCorr_hist : `dict`[`str`, `~numpy.ndarray`]
A dictionary containing the histogram values and bin lower/upper
edges for the skyCorr difference dataset.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also mids

defaultTemplates={"outputName": "deltaSkyCorr"},
):
data = ct.Input(
doc="tmp",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doc needs fixing



class DeltaSkyCorrXYPlot(AnalysisTool):
parametrizeBands = False
Copy link
Collaborator

@sr525 sr525 Dec 18, 2023

Choose a reason for hiding this comment

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

Do we really not want this to not be done in every band?

Copy link
Contributor

Choose a reason for hiding this comment

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

These are visit-level plot/metrics, so 1 visit = 1 band (i.e., there aren't multiple bands available).

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed in an in-person chat, and agreed out of scope for this ticket.

@leeskelvin leeskelvin force-pushed the tickets/DM-42043 branch 4 times, most recently from 64fb114 to 8143616 Compare December 22, 2023 20:05
Switch to ArrowNumpyDict storage class (LSK edits)

Add bin_mid to output

Add MedianHistAction; calc for deltaSkyCorr

Add IqrHistAction; change to line plot

Edits following review
@leeskelvin leeskelvin merged commit 7a8cb9f into main Jan 3, 2024
8 checks passed
@leeskelvin leeskelvin deleted the tickets/DM-42043 branch January 3, 2024 18:48
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.

None yet

4 participants