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

add imSim filter definitions #236

Merged
merged 10 commits into from
Aug 18, 2020
Merged

add imSim filter definitions #236

merged 10 commits into from
Aug 18, 2020

Conversation

jchiang87
Copy link
Contributor

No description provided.

@jchiang87 jchiang87 marked this pull request as ready for review July 30, 2020 22:06
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks good to me but I think we need an example header that will trigger this code. The example yaml header I have in the package doesn't seem to have a PKG header and the example data file doesn't either. It would be good if we could at least have a dumped yaml header added to the test_translator.py test.

@@ -29,7 +29,8 @@
from lsst.utils import getPackageDir
from lsst.obs.base import Instrument
from lsst.obs.base.gen2to3 import TranslatorFactory
from .filters import LSSTCAM_FILTER_DEFINITIONS, LATISS_FILTER_DEFINITIONS
from .filters import LSSTCAM_FILTER_DEFINITIONS, LATISS_FILTER_DEFINITIONS, \
Copy link
Member

Choose a reason for hiding this comment

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

We tend to prefer the form of:

from .filters import (
    LSSTCAM_FILTER_DEFINITIONS,
    LATISS_FILTER_DEFINITIONS,
    LSST_IMSIM_FILTER_DEFINITIONS,
)

for multi line rather than using a backslash.

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'm happy to change, but I just followed the .translators example below.

Copy link
Member

Choose a reason for hiding this comment

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

😄 you got me. I can do a clean up pass at some point. Leave it.

# data, we used throughputs version 1.4.
throughputs_version = None
for key, value in self._header.items():
if key.startswith('PKG') and value == "throughputs":
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit but this file consistently uses double quotes so could you please fix the single quotes?

@jchiang87
Copy link
Contributor Author

I'll add update the test_translator.py code to test these changes with a yaml header dump.

@timj
Copy link
Member

timj commented Aug 14, 2020

@jchiang87 if you rebase I can do a jenkins run if you want to get this merged?

@jchiang87
Copy link
Contributor Author

@timj Sure, I can do the rebase this afternoon. Is there a specific rebasing procedure that the DM team prefers?

@timj
Copy link
Member

timj commented Aug 14, 2020

Yes. Do not hit any buttons here convincing you to "update your branch"

$ git checkout master
$ git pull
$ git checkout u/jchiang/DM-26138
$ git rebase master
$ git push -f

@jchiang87
Copy link
Contributor Author

Done.

@timj
Copy link
Member

timj commented Aug 14, 2020

Test failed in some weird gen3 way so I'll have to take a look.

@timj
Copy link
Member

timj commented Aug 15, 2020

The Gen3 test is failing because there is no physical filter i and the test data we have in obs_lsst doesn't match the known filter list. The test file I'm using is:

Analyzing data/input/imsim/raw/204595/R11/00204595-R11-S20-det042.fits...
WARNING:lsst.obs.lsst.translators.imsim:204595: throughputs version not found.  Using FILTER keyword value 'i'.
instrument: LSST-ImSim
telescope: Simonyi Survey Telescope
datetime_begin: 2022-10-05T06:53:26.357
altaz_begin: <SkyCoord (AltAz: obstime=59857.28711061111, location=(1818938.94323602, -5208470.950938, -3195172.08426757) m, pressure=0.0 hPa, temperature=0.0 deg_C, relative_humidity=0.0, obswl=1.0 micron): (az, alt) in deg
    (93.25534842, 81.78122191)>
boresight_airmass: 1.0103771465719806
boresight_rotation_angle: 254.37566699731553 deg
boresight_rotation_coord: sky
dark_time: 30.0 s
datetime_end: 2022-10-05T06:53:56.357
detector_exposure_id: 204595042
detector_group: R11
detector_name: S20
detector_num: 42
detector_serial: LCA-11021_RTM-000
detector_unique_name: R11_S20
exposure_group: 204595
exposure_id: 204595
exposure_time: 30.0 s
location: (1818938.94323602, -5208470.950938, -3195172.08426757) m
object: UNKNOWN
observation_id: 204595
observation_type: science
physical_filter: i
pressure: None
relative_humidity: 40.0
science_program: 204595
temperature: None
tracking_radec: <SkyCoord (ICRS): (ra, dec) in deg
    (55.67759886, -30.44239357)>
visit_id: 204595

so which filter definitions would have been used for that?

@jchiang87
Copy link
Contributor Author

We've used the same filter defs for all DC2 sims. I've put a modern version of that file on lsst-dev in my home directory:

/home/jchiang/lsst_a_204595_R11_S20_i.fits

That file has the throughputs version keyword that will set the physical filter.

@timj
Copy link
Member

timj commented Aug 15, 2020

That new file does work and leads to gen2 registry including the new filter name. The downside for gen2 is that this means that the flats can't be found because they are still i. Fixing the CALIB_ID header in the flat file fixes the gen2 problem but because there is a . in the gen2 file name this breaks gen2 to 3 conversion because the . looks like a file extension to gen3 (which can't really tell the difference between .fits.gz and .4_xyz.fits from the 1.4. I'm not entirely sure what to do at this point (gen3 is fine once it has the file because it never allows . in an expanded template). The hacky answer is to call the new filter i_sim_1_4 without the .. What do you think?

Be aware that using proper physical filter names will mean you have to tweak all your reference flats to use the proper name.

@jchiang87
Copy link
Contributor Author

It sounds like there isn't an alternative to doing the hacky thing if we want to use the throughputs version number in the physical filter name, so we should just do that. Since there's precedent for replacing . with _, I guess this isn't so objectionable.

@timj
Copy link
Member

timj commented Aug 17, 2020

After discussion on Slack I'm going to fix butler ingest so we can keep the filter how we want it.

@timj timj merged commit 07a175c into master Aug 18, 2020
@timj timj deleted the u/jchiang/DM-26138 branch August 18, 2020 17:11
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

3 participants