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-11571 fixes for jointcal/meas_mosaic comparison output #65

Merged
merged 8 commits into from Dec 11, 2017

Conversation

parejkoj
Copy link
Contributor

@parejkoj parejkoj commented Dec 5, 2017

No description provided.

Copy link
Contributor

@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.

These seem reasonable. We will need to rebase the verify_port branch on these changes in order to get this functionality there. I don't think that needs to be done as part of this ticket, but should probably be captured in another ticket.

@@ -124,15 +124,15 @@ AM1:
- level: design
value: 10.0
unit: marcsec
filter_names: [z, r, i, HSC-R, HSC-I]
filter_names: [u, g, r, i, z, y, HSC-U, HSC-G, HSC-R, HSC-I, HSC-Z, HSC-Y]
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, with the verify port, this file has gone away, so we will need to port any changes here to verify_metrics. That doesn't need to be done here, but we will need to do it at some point.

@@ -173,6 +179,8 @@ def _loadAndMatchCatalogs(self, repo, dataIds, matchRadius,
calibration.
matchRadius : afwGeom.Angle(), optional
Radius for matching. Default is 1 arcsecond.
useJointCal : bool, optional
Use jointcal/meas_mosaic outputs to calibrate positions and fluxes.
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 this needs to document skipTEx as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, per your later comment, the argument is going away since we're going to use the class member.

# We don't want to put this above the first "if useJointCal block"
# because we need to use the first `butler.get` above to quickly
# catch data IDs with no usable outputs.
try:
# HSC supports these flags, which dramatically improve I/O
# performance; support for other cameras is DM-6927.
oldSrc = butler.get('src', vId, flags=SOURCE_IO_NO_FOOTPRINTS)
calexp = butler.get("calexp", vId, flags=SOURCE_IO_NO_FOOTPRINTS)
# calexp = butler.get("calexp", vId, flags=SOURCE_IO_NO_FOOTPRINTS)
Copy link
Contributor

Choose a reason for hiding this comment

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

If calexp isn't needed, this line should be deleted. Same with line 285.

tmpCat['e2'][:] = star_e2
tmpCat['psf_e1'][:] = psf_e1
tmpCat['psf_e2'][:] = psf_e2
if not self.skipTEx:
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I'm missing something, but you are accessing the skipTEx attribute on self which is set in the constructor. However, it is also legal to send a skipTEx parameter to this method. The two don't seem to be reconciled, which seems dangerous. I think you should just use either the attribute on self or pass it through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Good catch. Since this is an internal method that's called by __init__, I'm just going to use the class member and cleanup the method signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

@parejkoj parejkoj merged commit 9d45d43 into master Dec 11, 2017
@ktlim ktlim deleted the tickets/DM-11571 branch August 25, 2018 06:50
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