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-38563: Compare LATISS Defects generated from combined images #31

Merged
merged 15 commits into from Oct 26, 2023

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Oct 5, 2023

No description provided.

@plazas plazas requested a review from czwa October 5, 2023 20:01
name="camera",
storageClass="Camera",
doc="Input camera.",
dimensions=[
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not to concerned about which formatting style we use, but it should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree; this was probably a consequence of running black.

"UNC_MIN": "MIN",
"UNC_MAX": "MAX",
}
# initialize the dictionaries. They will get replaced.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add something about "These config options need to have a key/value pair to run verification analysis, but the contents of that pair are not used"? I'm sure I'll be confused next time I read this bit of code, and that should help.

ConfigClass = CpVerifyDefectsConfig
_DefaultName = 'cpVerifyDefects'
_DefaultName = "cpVerifyDefects"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for clearing up my inconsistent use of ' and ". It's the same as the formatting issue, and I know I'm consistently inconsistent.

if verifyStatsCat["SUCCESS"] is False:
successCat = False

success = successDet & successAmp & successCat
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced successAmp needs to be included, but if it doesn't cause problems, there's no reason to remove it, I think.

@@ -67,10 +75,11 @@ class CpVerifyStatsConnections(pipeBase.PipelineTaskConnections,
name="camera",
storageClass="Camera",
doc="Input camera.",
dimensions=["instrument", ],
dimensions=[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

plazas added 2 commits October 18, 2023 10:03
Add comment and fix formatting
Fix flake8 error
@plazas plazas merged commit e719232 into main Oct 26, 2023
3 checks passed
@plazas plazas deleted the tickets/DM-38563 branch October 26, 2023 18:23
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

2 participants