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-39592: Add support for Auxtel/LATISS and add associated tests. #112

Merged
merged 15 commits into from Sep 5, 2023

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Aug 29, 2023

This PR requires the latest version of fgcm, see lsst/fgcm#36


lsst.daf.butler.cli.cliLog.CliLog.initLog(longlog=False)

cls.testDir = tempfile.mkdtemp(dir=ROOT, prefix="TestFgcm-")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that this could clash with, e.g. https://github.com/lsst/fgcmcal/blob/main/tests/test_fgcmcal_hsc.py#L65
if tests are run concurrently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mkdtemp will add in a random 6 character string (including symbols), so the odds of clashing are pretty dang close to zero.

os.path.join(cls.dataDir, 'latiss', 'exports.yaml'))

def test_fgcmcalPipeline(self):
"""Test running the full pipeline, using new isolated star association code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Still "new"?

"band IN ('g', 'r', 'i')",
visits,
zpOffsets,
refcatCollection="refcats/DM-33444",
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't work until after a0c0181

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's on this ticket, right? That's why I added that... (I guess I don't know what this comment is exactly referring to).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry…meant to delete that one (I hadn’t been looking commit by commit & messed up the order in my mind 🤪)

0,
'r',
1,
testSrc=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required here but not for the associated test_fgcmcal_hsc.py version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without completely rewriting this part of the test functionality, I would have to import the src files into the test repo (which I don't want to do). And I think that the code coverage is there from the hsc test harness.


config.filterTransmissionIsUnbounded = False
config.opticsTransmissionIsUnbounded = False
config.sensorTransmissionIsUnbounded = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just leave these out now?

config.sourceSelector["science"].signalToNoise.fluxField = "apFlux_35_0_instFlux"
config.sourceSelector["science"].signalToNoise.errField = "apFlux_35_0_instFluxErr"
config.fgcmLoadReferenceCatalog.load(os.path.join(configDir, 'filterMapLatiss.py'))
config.fgcmLoadReferenceCatalog.applyColorTerms = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be set to True as of DM-40556?

config.washMjds = (0.0, )
# For tests, define 1 observing epoch that encompasses everything.
config.epochMjds = (0.0, 100000.0)
config.coatingMjds = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Hhmmm...how does this differ from the default of default=(0.0,)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You really want to define the start and end points, and it logs a warning if you don't; I have the default be something that should be overridden.

config.referencePixelizationMinStars = 20
config.referenceMinMatch = 15

from lsst.obs.lsst.filters import LATISS_FILTER_DEFINITIONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you not use this (rather than set explicitly below)?

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 only want to define the set of filters that we're using, rather than all possible filters.

'SDSSi_65mm~empty': 'i',
}

config.photoCal.applyColorTerms = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you turn this on now (as of DM-40556)?

instrument: lsst.obs.lsst.Latiss
tasks:
fgcmOutputProducts:
class: lsst.fgcmcal.fgcmOutputProducts.FgcmOutputProductsTask
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like all these pipelines differ from the HSC versions only by the imported instrument. Would making a bas e (or ingredients file?) make sense/be worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe when I add a third test camera, but with 2 it's definitely not worth it (yet) to make a whole new directory structure just for test configs.

@erykoff erykoff merged commit 2ab4ef6 into main Sep 5, 2023
2 checks passed
@erykoff erykoff deleted the tickets/DM-39592 branch September 5, 2023 19:52
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