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-32207: Add physical_filter to metrics dimensions. #200

Merged
merged 1 commit into from Oct 12, 2021

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Oct 12, 2021

No description provided.

Copy link

@SimonKrughoff SimonKrughoff left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of minor questions

@@ -147,6 +147,15 @@ def check_output(visit, detectors):
check_output(34648, [51, 59, 67])
check_output(34690, [48, 56, 64])

def test_jointcalTask_r_visits_2_bands_simple_gen3_dm32207(self):

Choose a reason for hiding this comment

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

I'm not sure what r_visits is saying here. If both visits are r-band, I think it will trigger the error, but the name and doc string are ambiguous to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't even notice that, just copypasta.

@@ -294,43 +294,43 @@ class JointcalTaskConnections(pipeBase.PipelineTaskConnections,
doc=f"The number of cross-matched fittedStars for {name}",
name=f"metricvalue_jointcal_{name}_matched_fittedStars",
storageClass="MetricValue",
dimensions=("instrument", "skymap", "tract"),
dimensions=("skymap", "tract", "instrument", "physical_filter"),

Choose a reason for hiding this comment

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

Out of curiosity, why the change in order? Do we have a canonical order for listing dimensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the order to match the quantum dimensions. I don't think it matters, I hope.

@erykoff erykoff merged commit bbdf43d into master Oct 12, 2021
@erykoff erykoff deleted the tickets/DM-32207 branch October 12, 2021 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants