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-12882: Use compression-safe method for getting calexp dimensions. #23

Merged
merged 1 commit into from Dec 2, 2017

Conversation

TallJimbo
Copy link
Member

No description provided.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

I'm not the official reviewer, but this looks like an improvement to me. I had some comments, though they may be on a slightly outdated version of the code.

catalog = mosaicUtils.rotatePixelCoordsBack(catalog, calexp_md.get("NAXIS1"),
calexp_md.get("NAXIS2"), nQuarter)
catalog = mosaicUtils.rotatePixelCoordsBack(catalog, dimensions.getX(),
dimensions.getY(), nQuarter)
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this code violates linting all over the place, but can you do anything to make sure it won't complain about dimensions possibly not being defined here? If it's too expensive to always get dimensions even if it's not needed, then you could set it to None before the first if nQuarter%4...

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran a linter on this file and found one real error which I will ask you to fix while you're in here: TaskError is not defined. Please add it to the items imported form lsst.pipe.base at line 26.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function getFluxFitParams gets wcsHeader but does not use it. I suggest deleting the two lines that get it. I suspect this may be leftover

Copy link
Contributor

Choose a reason for hiding this comment

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

The function getWcs reads metadata and then uses it to construct a WCS. Would it work to read the WCS directly?:

    if hscRun is not None:
       wcsName = "wcs_hsc"
    else:
        wcsName = "wcs"
    return dataRef.get(wcsName, immediate=True)

Copy link
Member Author

@TallJimbo TallJimbo Dec 2, 2017

Choose a reason for hiding this comment

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

Unfortunately the wcs dataset is actually an Exposure dataset, so at best we could do dataRef.get("wcs").getWcs() - but I think even that doesn't work because it's a zero-size Exposure.

We should make it actually a Wcs (or soon, SkyWcs) persisted directly with FitsCatalogStorage (DM-11138). I think that'll have to be backwards incompatible, however, and I haven't had the free energy to RFC that and push it through.

@ktlim ktlim deleted the tickets/DM-12882 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