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-6652: Remove database hack #5

Merged
merged 10 commits into from Oct 31, 2016
Merged

DM-6652: Remove database hack #5

merged 10 commits into from Oct 31, 2016

Conversation

PaulPrice
Copy link
Contributor

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.

I don't have data to check this on, but if things have been fixed in the headers it looks like these are all good changes.

# we don't have astrometry_net data (yet) so astrom and photo cal are impossible
config.doCalibrate=False
else:
elif False:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any need to leave this in, if we don't do calibration now anyway? We can always add it back in when we do the calibration step, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a convenience for the user. A dirty convenience I grant you, but the whole package is dirty...

@@ -37,3 +38,7 @@
config.calibrate.astrometry.solver.useWcsRaDecCenter = False # It's off for some reason dunno yet
config.calibrate.astrometry.solver.useWcsParity = False # I doubt I guess right
config.calibrate.astrometry.solver.useWcsPixelScale = False # DGM says it's 0.4, but....
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Were there actually Sloan filters available? Same as above, it seems like master should have a clean config and we can add on in a branch as new features are available.

@@ -6,6 +6,10 @@
config.isr.doFringe = False
config.isr.doLinearize = False

config.charImage.repair.doCosmicRay = False
config.charImage.repair.cosmicray.nCrPixelMax = 1000000
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to set nCrPixelMax if doCosmicRay is False.

@@ -84,6 +86,10 @@ def run(self, ccdExposure, bias=None, dark=None, flat=None, defects=None, fring
ampExposure = ccdExposure.Factory(ccdExposure, amp.getBBox())
self.updateVariance(ampExposure, amp)

# Don't trust the variance not to be negative (over-subtraction of dark?)
variance = ccdExposure.getMaskedImage().getVariance().getArray()
variance[:] = numpy.where(variance > 0, variance, 100.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the value 100 motivated? Seems like it could be configurable or calculated from the data.

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'll try setting it using a robust stdev of the image.

@@ -1,4 +1,4 @@
from lsst.obs.monocam import MonocamIsrTask
config.isr.retarget(MonocamIsrTask)
config.dateObs = "date"
config.darkTime = None
config.darkTime = "DARKTIME"
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this now comes in the VisitInfo, but that's for another ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was written quite a while ago, and hasn't been modernised.

Now that the FITS files have been "sanitized" (updated with the appropriate
metadata), the hack is not necessary, but it actually gets in the way
because the database connection prevents use of multiprocessing.
Might be useful for selecting calibs when combining.
This allows us to work with a modern ip_isr, following DM-5462.
It's now available in the sanitized FITS files.
We have biases and darks, so activate the correction that uses these.
I'm concerned that this can oversubtract sky data, and so added a guard
against having negative variance.
There may be something in the calibs that causes us to find a LOT of CRs,
so disable that until we can figure out what it is.
Merlin says that the noise problems have been fixed.
It gives better PSF models and is more robust than our PcaPsf.
Until astrometric matching performance can be verified.
@PaulPrice PaulPrice merged commit ec81aa7 into master Oct 31, 2016
@ktlim ktlim deleted the tickets/DM-6652 branch August 25, 2018 06:15
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