Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: obs_subaru
-
Labels:
-
Story Points:6
-
Epic Link:
-
Sprint:Science Pipelines DM-S15-1, Science Pipelines DM-S15-2, Science Pipelines DM-S15-3, Science Pipelines DM-S15-4, Science Pipelines DM-S15-5, Science Pipelines DM-S15-6
-
Team:Data Release Production
Description
Reducing HSC data requires an estimate of the distortion, which is provided by the HSC package distEst. This can be pulled into obs_subaru to consolidate code and reduce dependencies.
I propose to treat distEst as legacy code, which means I will pull it into obs_subaru without major changes to the code style.
Attachments
Issue Links
- is blocked by
-
DM-245 Implement HSC camera in new camera framework
- Done
- is duplicated by
-
DM-2796 obs_subaru cannot be built due to a missing hsc header file
- Won't Fix
-
DM-2194 Ensure proper functioning of HSC distortion correction within obs_subaru
- Invalid
- relates to
-
DM-245 Implement HSC camera in new camera framework
- Done
Activity
I've done this, but am holding off requesting a review until DM-245 is done, since it is built on top of u/lauren/DM-245.
commit 3590678c6e66fbab61aadba9ed66c7a74577ed73
|
Merge: de069f3 eea7699
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 12:14:39 2015 -0500
|
|
Merge distEst package into obs_subaru (DM-1794)
|
|
Pulled distEst from hsc-repo.mtk.nao.ac.jp:repositories/distEst.git
|
|
Conflicts:
|
.gitignore
|
SConstruct
|
lib/SConscript
|
tests/SConscript
|
|
commit 8c6decae1efa8da8ea70acf4c573b0db3f5603d5
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 12:39:37 2015 -0500
|
|
Reorganise files following merge of distEst.
|
|
include/hsc/meas/match/LeastSquares.h | 18 ----
|
include/hsc/meas/match/distest.h | 22 -----
|
include/hsc/meas/match/distest2.h | 17 ----
|
include/hsc/meas/match/distest_utils2.h | 32 -------
|
include/lsst/obs/hsc/LeastSquares.h | 18 ++++
|
include/lsst/obs/hsc/distest.h | 22 +++++
|
include/lsst/obs/hsc/distest2.h | 17 ++++
|
include/lsst/obs/hsc/distest_utils2.h | 32 +++++++
|
python/hsc/__init__.py | 2 -
|
python/hsc/meas/__init__.py | 2 -
|
python/hsc/meas/match/SConscript | 3 -
|
python/hsc/meas/match/__init__.py | 0
|
python/hsc/meas/match/distest.i | 40 --------
|
python/hsc/meas/match/hscDistortion.py | 56 -----------
|
python/lsst/obs/hsc/distest.i | 40 ++++++++
|
src/distest_HSCSIM.cc | 164 --------------------------------
|
16 files changed, 129 insertions(+), 356 deletions(-)
|
|
commit 1f634ec943812b4aaa5f34848a7356f25065b1e5
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 12:50:02 2015 -0500
|
|
Prepare distEst for building within obs_subaru.
|
|
Changed namespace from hsc::meas::match to lsst::obs::hsc. Moved
|
code that wasn't in any namespace into the lsst::obs::hsc namespace.
|
|
include/lsst/obs/hsc/LeastSquares.h | 15 +++---
|
include/lsst/obs/hsc/distest.h | 22 ++++-----
|
include/lsst/obs/hsc/distest2.h | 11 ++++-
|
include/lsst/obs/hsc/distest_utils2.h | 10 +++-
|
python/lsst/obs/hsc/SConscript | 6 ---
|
python/lsst/obs/hsc/distest.i | 14 +++---
|
python/lsst/obs/hsc/hscLib.i | 23 ---------
|
src/DistEstXYTransform.cc | 8 ++--
|
src/LeastSquares.cc | 8 ++++
|
src/distest.cc | 10 ++--
|
src/distest2.cc | 14 ++++++
|
src/distest_utils2.cc | 87 ++++-------------------------------
|
ups/distEst.build | 2 -
|
ups/distEst.cfg | 15 ------
|
ups/distEst.table | 8 ----
|
15 files changed, 79 insertions(+), 174 deletions(-)
|
|
commit afccc5f30fc7b914e9a73c7f4535a00e93a4481e
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 14:59:53 2015 -0500
|
|
hsc: Fix dependency in building defects
|
|
Using the python scripts to build defects requires that the
|
SWIG module has been built first.
|
|
hsc/SConscript | 6 ++++--
|
1 file changed, 4 insertions(+), 2 deletions(-)
|
|
commit faf14f39840d7d8433ec99c97704457cbfbfcb68
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 15:02:09 2015 -0500
|
|
Updates to allow package to build with LSST stack
|
|
include/lsst/obs/subaru/DistEstXYTransform.h | 1 +
|
src/distest2.cc | 411 ++++-----------------------
|
src/distest_utils2.cc | 4 +-
|
3 files changed, 52 insertions(+), 364 deletions(-)
|
Lauren, where does this stand after the merge of DM-245?
Paul Price I pulled down your branch and merged it into the latest DM-245 branch (it had changed A LOT since your pull for this work). There were quite a few conflicts to resolve, but all seems OK. I've pushed this into a u/lauren/DM-1794 branch, so you can have a look and feel free to finish this ticket off (or let me know if you'd like me to do the merge once you've had a look).
I also plan to make a new issue along the lines of making sure distEst is getting passed numbers in the proper units (without messing around with the camera.py representation which would result in inconsistencies with the other obs_XXX representations).
I'd like to ask Lauren to review this, but because it's her first time I've added John as a back-up. I don't think it's essential to get this in before S15 starts, so please don't kill yourselves getting this done over the weekend. I don't think I can test this via buildbot, so Lauren would you please double-check it builds with the stack you've been using?
The code on tickets/DM-1794 contains many commits, but I'm listing only the top 5 below because they're the work that I've done to integrate distEst into obs_subaru. I preserved the history of distEst just in case we have to go digging in the future, but the commit discipline in that package is not up to the current LSST standard. This is one of those rare cases where I don't think you should review every commit, but mostly focus on the finished product.
price@price-laptop:~/LSST/obs/subaru (tickets/DM-1794 %=) $ git --no-pager log --stat -5
|
commit 64d84071652e2737e3e5b255c3fc844252f27d4f
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 15:02:09 2015 -0500
|
|
Updates to allow package to build with LSST stack
|
|
include/lsst/obs/subaru/DistEstXYTransform.h | 1 +
|
src/distest2.cc | 411 ++++-----------------------
|
src/distest_utils2.cc | 4 +-
|
3 files changed, 52 insertions(+), 364 deletions(-)
|
|
commit 3502650cd2c8f54929c3faeff2fbb16c638a2e8b
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 14:59:53 2015 -0500
|
|
hsc: Fix dependency in building defects
|
|
Using the python scripts to build defects requires that the
|
SWIG module has been built first.
|
|
hsc/SConscript | 4 +++-
|
1 file changed, 3 insertions(+), 1 deletion(-)
|
|
commit c32b147f3917b7cb2bb9dba7117c2f62b2a6b93c
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 12:50:02 2015 -0500
|
|
Prepare distEst for building within obs_subaru.
|
|
Changed namespace from hsc::meas::match to lsst::obs::hsc. Moved
|
code that wasn't in any namespace into the lsst::obs::hsc namespace.
|
|
include/lsst/obs/hsc/LeastSquares.h | 15 +++---
|
include/lsst/obs/hsc/distest.h | 22 ++++-----
|
include/lsst/obs/hsc/distest2.h | 11 ++++-
|
include/lsst/obs/hsc/distest_utils2.h | 10 +++-
|
python/lsst/obs/hsc/distest.i | 14 +++---
|
src/DistEstXYTransform.cc | 8 ++--
|
src/LeastSquares.cc | 8 ++++
|
src/distest.cc | 10 ++--
|
src/distest2.cc | 14 ++++++
|
src/distest_utils2.cc | 87 ++++-------------------------------
|
ups/obs_subaru.table | 1 -
|
11 files changed, 79 insertions(+), 121 deletions(-)
|
|
commit 69197ea5246a3803ea371d6a03846615bd8a803b
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Wed Jan 21 12:39:37 2015 -0500
|
|
Reorganise files following merge of distEst.
|
|
include/hsc/meas/match/LeastSquares.h | 18 ----
|
include/hsc/meas/match/distest.h | 22 -----
|
include/hsc/meas/match/distest2.h | 17 ----
|
include/hsc/meas/match/distest_utils2.h | 32 -------
|
include/lsst/obs/hsc/LeastSquares.h | 18 ++++
|
include/lsst/obs/hsc/distest.h | 22 +++++
|
include/lsst/obs/hsc/distest2.h | 17 ++++
|
include/lsst/obs/hsc/distest_utils2.h | 32 +++++++
|
python/hsc/meas/match/distest.i | 40 --------
|
python/hsc/meas/match/hscDistortion.py | 56 -----------
|
python/lsst/obs/hsc/distest.i | 40 ++++++++
|
src/distest_HSCSIM.cc | 164 --------------------------------
|
12 files changed, 129 insertions(+), 349 deletions(-)
|
|
commit a4092c6219affdb54253cfef6a17135cde901d0c
|
Merge: 44fc8df 39e3fed
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Fri Feb 27 16:36:56 2015 -0500
|
|
Merge distEst package into obs_subaru
|
|
Pulled distEst from hsc-repo.mtk.nao.ac.jp:repositories/distEst.git
|
|
distEst is a program from the HSC software team for estimating the
|
distortion in HSC. It was its own package because it was originally
|
used independently of obs_subaru to create simulated HSC images.
|
We're now pulling it into obs_subaru because it no longer has any
|
utility as a stand-alone package, and the distortion correction is
|
essential for processing HSC data.
|
|
price@price-laptop:~/LSST/obs/subaru (tickets/DM-1794 %=) $ git --no-pager diff --stat origin/master...
|
example/DISTforSIM.cc | 2863 +++++++++++++++++++++++
|
example/DISTforSIM.h | 33 +
|
example/display.py | 79 +
|
example/distEst2.py | 38 +
|
example/example.py | 23 +
|
example/example2.py | 26 +
|
hsc/SConscript | 4 +-
|
include/lsst/obs/hsc/LeastSquares.h | 17 +
|
include/lsst/obs/hsc/distest.h | 16 +
|
include/lsst/obs/hsc/distest2.h | 24 +
|
include/lsst/obs/hsc/distest_utils2.h | 38 +
|
include/lsst/obs/subaru/DistEstXYTransform.h | 1 +
|
python/lsst/obs/hsc/distest.i | 40 +
|
src/DistEstXYTransform.cc | 8 +-
|
src/LeastSquares.cc | 296 +++
|
src/distest.cc | 164 ++
|
src/distest2.cc | 810 +++++++
|
src/distest_utils2.cc | 3204 ++++++++++++++++++++++++++
|
tests/invariant.py | 109 +
|
ups/obs_subaru.table | 1 -
|
20 files changed, 7788 insertions(+), 6 deletions(-)
|
For posterity, the command I used to prepare distEst for merging was:
git filter-branch \
|
--prune-empty \
|
--index-filter '
|
git ls-tree -z -r --name-only --full-tree $GIT_COMMIT \
|
| grep -z -v "^include/" \
|
| grep -z -v "^python/hsc/meas/match/distest.i$" \
|
| grep -z -v "^python/hsc/meas/match/hscDistortion.py$" \
|
| grep -z -v "^src/" \
|
| grep -z -v "^example/" \
|
| grep -z -v "^tests/invariant.py$" \
|
| xargs -0 git rm --cached -r
|
' \
|
-- \
|
--all
|
Then, in obs_subaru I did git fetch /path/to/distEst; git merge FETCH_HEAD. Then I added the above 5 commits to clean things up.
Sorry – JIRA didn't mail me about this, so I've only just seen that you assigned me as a reviewer. Not going to get this done in W15; sorry. I'll bump to the next cycle, and take a look at it shortly.
I looked this over briefly earlier. I understand the comment that the commit structure isn't what we'd hope for on LSST, but, even given that, my impression was that the code itself could use a lot more work to comply with LSST conventions.
I'm leery of investing too much time that we've not budgeted for in this code, if it's fundamentally adequate to get HSC data flowing through the LSST pipeline and will only ever be of interest to a minority of folks (most of whom are probably already happily using the HSC version). However, I'm also unhappy with just merging something that's fairly flagrantly violating a bunch of LSST conventions into the LSST repository; I'm not sure we can avoid this by just branding it 'legacy'.
We discussed this a little on HipChat, and didn't really reach a conclusion. Further thoughts welcome.
It's not a great deal of code, cleanup is easier than writing new code, and it would be good to have it cleaned up, so I'll give it a go. Is it worth preserving the history if everything's going to get cleaned up?
Generally my attitude would be that most obs_ packages should be treated as something like contributions and not necessarily required to meet LSST standards. But if cleaning it up is possible, I think it's better to do it.
Paul Price, if you're willing to spend a bit of time on cleanup that would be great! I wouldn't let this become a significant time sink, though, especially given K-T's comment. Personally, I don't think preserving the HSC history is essential.
Kian-Tat Lim, that attitude is basically what I was hoping for. But: it seems likely that once we've got most of the HSC changes merged back to LSST, we'll want to use obs_subaru not just for the entertainment of scientists but also as part of integration testing for the LSST stack. Does that change the position?
We discussed this in today's coordination standup. The consensus is that we should enforce LSST's coding standards for things going into obs_subaru and, indeed, any other packages obs_ which will be a requirement for LSST's testing and development. Other obs_ packages which are judged to not be of direct importance to LSST may be regarded as "contrib" or similar and hence not held to the same standards.
Since this is work required by LSST, it's of course appropriate for LSST to pay for it – that is, we'd expect this effort to come out of Paul Price's (in this case) LSST time. Paul, I get the impression from your comment above that it's not a huge job, but feel free to create a new story or add more story points to this one to cover the effort you'll be putting in, and do scream if it's something you can't see yourself having time for in the foreseeable future due to your HSC commitments.
I think I've broken the back of this... Hope to have it polished up soon.
John Swinbank, would you please have a look at this? Lauren MacArthur, you may want to look too, but I realise I just gave you a different review.
There are several commits on tickets/DM-1794 of obs_subaru. Most are fixes of little things that I noticed, but the big one is 111d2ea, which imports the functionality of distEst into obs_subaru.
price@price-laptop:~/LSST/obs/subaru (tickets/DM-1794=) $ git sub
|
commit 59f92bb9cd081b79b52dd3f7eee96c1c2560a57c
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 19:53:55 2015 -0400
|
|
genDefectFits.py: fix instantiation of mapper
|
|
bin/genDefectFits.py | 2 +-
|
1 file changed, 1 insertion(+), 1 deletion(-)
|
|
commit a2780b563992a64a5d51537d620fde62bf2355e5
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 19:54:32 2015 -0400
|
|
registryInfo.py: don't require postgresql
|
|
bin/registryInfo.py | 3 ++-
|
1 file changed, 2 insertions(+), 1 deletion(-)
|
|
commit 0302788e7feb5473b1be05e5dd093de6ceca410c
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 19:56:06 2015 -0400
|
|
require SWIGed module before building defects registry
|
|
hsc/SConscript | 4 +++-
|
1 file changed, 3 insertions(+), 1 deletion(-)
|
|
commit 555767b59ad08c5e307437c1ed8b7ffcc13065f1
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:03:30 2015 -0400
|
|
genCameraRepository.py: convert to a plate scale of 1
|
|
LSST likes to use mm/pixels, but Subaru works in nominal pixels from the
|
boresite, or a plate scale of 1. This is true for both Suprime-Cam and
|
HSC.
|
|
This does not include updates to the camera definition files, just the
|
conversion.
|
|
bin/genCameraRepository.py | 6 ++----
|
1 file changed, 2 insertions(+), 4 deletions(-)
|
|
commit 6a7fd49769570367718e0af10ecbd28b8664b92a
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:07:46 2015 -0400
|
|
fix amp names
|
|
Subaru amplifiers aren't indexed in two dimensions, so no need for an
|
extra number that's always zero.
|
|
bin/genCameraRepository.py | 2 +-
|
python/lsst/obs/subaru/isr.py | 5 +----
|
2 files changed, 2 insertions(+), 5 deletions(-)
|
|
commit 111d2ea907e8893557e5ccef9e3e155eb265c4ac
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:15:35 2015 -0400
|
|
add HSC distortion model rather than using external package distEst
|
|
This implements distEst using the new cameraGeom framework, and activates
|
the new distortion model.
|
|
bin/genCameraRepository.py | 5 +-
|
hsc/camera/0_00.fits | 4 +-
|
hsc/camera/0_01.fits | 6 +-
|
hsc/camera/0_02.fits | 6 +-
|
hsc/camera/0_03.fits | 4 +-
|
hsc/camera/0_04.fits | 4 +-
|
hsc/camera/0_05.fits | 2 +-
|
hsc/camera/0_06.fits | 6 +-
|
hsc/camera/0_07.fits | 4 +-
|
hsc/camera/0_08.fits | 4 +-
|
hsc/camera/0_09.fits | 4 +-
|
hsc/camera/0_10.fits | 8 +-
|
hsc/camera/0_11.fits | 2 +-
|
hsc/camera/0_12.fits | 4 +-
|
hsc/camera/0_13.fits | 2 +-
|
hsc/camera/0_14.fits | 6 +-
|
hsc/camera/0_15.fits | 2 +-
|
hsc/camera/0_16.fits | 2 +-
|
hsc/camera/0_17.fits | 6 +-
|
hsc/camera/0_18.fits | 4 +-
|
hsc/camera/0_19.fits | 6 +-
|
hsc/camera/0_20.fits | 2 +-
|
hsc/camera/0_21.fits | 2 +-
|
hsc/camera/0_22.fits | 4 +-
|
hsc/camera/0_23.fits | 4 +-
|
hsc/camera/0_24.fits | 4 +-
|
hsc/camera/0_25.fits | 4 +-
|
hsc/camera/0_26.fits | 4 +-
|
hsc/camera/0_27.fits | 2 +-
|
hsc/camera/0_28.fits | 4 +-
|
hsc/camera/0_29.fits | 4 +-
|
hsc/camera/0_30.fits | 2 +-
|
hsc/camera/0_31.fits | 2 +-
|
hsc/camera/0_32.fits | 2 +-
|
hsc/camera/0_33.fits | 4 +-
|
hsc/camera/0_34.fits | 4 +-
|
hsc/camera/0_35.fits | 2 +-
|
hsc/camera/0_36.fits | 4 +-
|
hsc/camera/0_37.fits | 8 +-
|
hsc/camera/0_38.fits | 4 +-
|
hsc/camera/0_39.fits | 2 +-
|
hsc/camera/0_40.fits | 4 +-
|
hsc/camera/0_41.fits | 2 +-
|
hsc/camera/0_42.fits | 6 +-
|
hsc/camera/0_43.fits | 6 +-
|
hsc/camera/0_44.fits | 4 +-
|
hsc/camera/0_45.fits | 2 +-
|
hsc/camera/0_46.fits | 4 +-
|
hsc/camera/0_47.fits | 6 +-
|
hsc/camera/0_48.fits | 4 +-
|
hsc/camera/0_51.fits | 4 +-
|
hsc/camera/0_52.fits | 4 +-
|
hsc/camera/0_53.fits | 6 +-
|
hsc/camera/0_54.fits | 6 +-
|
hsc/camera/0_55.fits | 2 +-
|
hsc/camera/0_56.fits | 2 +-
|
hsc/camera/0_57.fits | 6 +-
|
hsc/camera/1_00.fits | 4 +-
|
hsc/camera/1_01.fits | 2 +-
|
hsc/camera/1_02.fits | 4 +-
|
hsc/camera/1_03.fits | 4 +-
|
hsc/camera/1_04.fits | 4 +-
|
hsc/camera/1_05.fits | 2 +-
|
hsc/camera/1_06.fits | 2 +-
|
hsc/camera/1_07.fits | 4 +-
|
hsc/camera/1_08.fits | 6 +-
|
hsc/camera/1_09.fits | 6 +-
|
hsc/camera/1_10.fits | 2 +-
|
hsc/camera/1_11.fits | 8 +-
|
hsc/camera/1_12.fits | 4 +-
|
hsc/camera/1_13.fits | 4 +-
|
hsc/camera/1_14.fits | 4 +-
|
hsc/camera/1_15.fits | 2 +-
|
hsc/camera/1_16.fits | 6 +-
|
hsc/camera/1_17.fits | 6 +-
|
hsc/camera/1_18.fits | 2 +-
|
hsc/camera/1_19.fits | 2 +-
|
hsc/camera/1_20.fits | 2 +-
|
hsc/camera/1_21.fits | 2 +-
|
hsc/camera/1_22.fits | 2 +-
|
hsc/camera/1_23.fits | 2 +-
|
hsc/camera/1_24.fits | 2 +-
|
hsc/camera/1_25.fits | 2 +-
|
hsc/camera/1_26.fits | 2 +-
|
hsc/camera/1_27.fits | 4 +-
|
hsc/camera/1_28.fits | 4 +-
|
hsc/camera/1_29.fits | 2 +-
|
hsc/camera/1_30.fits | 6 +-
|
hsc/camera/1_31.fits | 6 +-
|
hsc/camera/1_32.fits | 2 +-
|
hsc/camera/1_33.fits | 4 +-
|
hsc/camera/1_34.fits | 2 +-
|
hsc/camera/1_35.fits | 4 +-
|
hsc/camera/1_36.fits | 4 +-
|
hsc/camera/1_37.fits | 2 +-
|
hsc/camera/1_38.fits | 4 +-
|
hsc/camera/1_39.fits | 2 +-
|
hsc/camera/1_40.fits | 2 +-
|
hsc/camera/1_41.fits | 4 +-
|
hsc/camera/1_42.fits | 2 +-
|
hsc/camera/1_43.fits | 4 +-
|
hsc/camera/1_44.fits | 4 +-
|
hsc/camera/1_45.fits | 4 +-
|
hsc/camera/1_46.fits | 2 +-
|
hsc/camera/1_47.fits | 4 +-
|
hsc/camera/1_48.fits | 4 +-
|
hsc/camera/1_51.fits | 6 +-
|
hsc/camera/1_52.fits | 4 +-
|
hsc/camera/1_53.fits | 4 +-
|
hsc/camera/1_54.fits | 4 +-
|
hsc/camera/1_55.fits | 6 +-
|
hsc/camera/1_56.fits | 6 +-
|
hsc/camera/1_57.fits | 6 +-
|
hsc/camera/camera.py | 940 ++++++++++++++++++++++++++++++------------------------------
|
include/lsst/obs/subaru/DistEstXYTransform.h | 62 ----
|
include/lsst/obs/subaru/HscDistortion.h | 91 ++++++
|
python/lsst/obs/hsc/transformRegistry.py | 271 ++++++++++++++++--
|
python/lsst/obs/subaru/subaruLib.i | 6 +-
|
src/DistEstXYTransform.cc | 63 ----
|
src/HscDistortion.cc | 139 +++++++++
|
tests/hscDistortion.py | 119 ++++++++
|
ups/obs_subaru.cfg | 11 +-
|
ups/obs_subaru.table | 1 -
|
123 files changed, 1294 insertions(+), 840 deletions(-)
|
|
commit b6aaa081fdf6b8522274ba94ab7cafe76cad6fa0
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:16:32 2015 -0400
|
|
generate new camera definitions for Suprime-Cam
|
|
suprimecam/camera/Chihiro.fits | 4 ++--
|
suprimecam/camera/Clarisse.fits | 6 ++---
|
suprimecam/camera/Fio.fits | 2 +-
|
suprimecam/camera/Kiki.fits | 6 ++---
|
suprimecam/camera/Nausicaa.fits | 6 ++---
|
suprimecam/camera/Ponyo.fits | 4 ++--
|
suprimecam/camera/San.fits | 4 ++--
|
suprimecam/camera/Satsuki.fits | 2 +-
|
suprimecam/camera/Sheeta.fits | 4 ++--
|
suprimecam/camera/Sophie.fits | 6 ++---
|
suprimecam/camera/camera.py | 88 ++++++++++++++++++++++++++++++++++------------------------------------
|
suprimecam/mit_camera/camera.py | 90 ++++++++++++++++++++++++++++++++++--------------------------------------
|
suprimecam/mit_camera/si001s.fits | 2 +-
|
suprimecam/mit_camera/si002s.fits | 2 +-
|
suprimecam/mit_camera/si005s.fits | 2 +-
|
suprimecam/mit_camera/si006s.fits | 2 +-
|
suprimecam/mit_camera/w4c5.fits | 2 +-
|
suprimecam/mit_camera/w67c1.fits | 2 +-
|
suprimecam/mit_camera/w6c1.fits | 2 +-
|
suprimecam/mit_camera/w7c3.fits | 2 +-
|
suprimecam/mit_camera/w93c2.fits | 2 +-
|
suprimecam/mit_camera/w9c2.fits | 2 +-
|
22 files changed, 117 insertions(+), 125 deletions(-)
|
|
commit b9e6c60db15d415a3ed76196d32b2b35017c0242
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:16:58 2015 -0400
|
|
cleanup lib/Sconscript
|
|
lib/SConscript | 2 --
|
1 file changed, 2 deletions(-)
|
|
commit eee651a591ca300ca5524f1c5543cbae1b9dd8c3
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:18:03 2015 -0400
|
|
add image of CCD positions for HSC
|
|
This is useful for debugging.
|
|
Taken from http://subarutelescope.org/Observing/Instruments/HSC/CCDPosition_20140811.png
|
|
hsc/CCDPosition_20140811.png | Bin 0 -> 370956 bytes
|
1 file changed, 0 insertions(+), 0 deletions(-)
|
Thanks Paul – I'll look at this as soon as I get a moment. Should be within a couple of days.
To start, I should apologise — this is my first time looking in any detail at an obs_ repository, and I suspect some things I’ll query are a matter of established convention rather than an innovation you’ve introduced on this ticket. I hope you’ll bear with me.
A couple of very minor general comments:
- Our guidelines suggest that summary line in commit messages should be capitalized.
- I note you often include a filename in the summary (genDefectFits.py: fix instantiation of mapper, etc). I have no serious objection to this, but I do wonder how helpful it is given that the files may later be renamed, or even your patch may apply somewhere quite different if it ever gets cherry-picked.
Now some per-commit queries:
- 59f92b
- By my reading, root=“.” is the default in the general CameraMapper class, but the obs_subaru mappers fall back to a SUPRIME_DATA_DIR environment variable instead. Except… here you’ve over-riding that fallback. I’m sure there are good reasons, but maybe a comment would be a appropriate to explain why?
- a2780b
- Looks like PgSqlConfig is currently only exists on HSC. Is that something we need to bring over to LSST?
- 030278
- I don’t understand what this does. I revert this commit, everything still builds fine when I run scons in the top level directory. Even with this commit included, it breaks if I just run scons hsc (ImportError: No module named subaruLib). So what’s this for?
- 555767
- Having defined PIXELSIZE at line 17, why not use that constant at line 83?
- 3065f1
- Fine, no comments.
- 111d2e:
- Line 47 of HscDistortion.cc is (just!) too long.
- The big chunk of pickle in tests/hscDistortion.py makes me a bit uncomfortable: it’s big and opaque and hard to diff (not, I suppose, that you expect it to change in future). Did you consider alternative representations? There are “only” 112 CCDs represented in total: simply directly constructing them in Python code doesn’t sound impossible.
- The test case you’ve provided is nice, in that it demonstrates your package really does replicate the functionality in HSC’s distEst. But it’s really quite a high-level sort of verification test, rather than a direct unit test of the HscDistortion functionality. Would it be worth including some simple tests where you directly invoke HscDistortion and demonstrate the basic functionality?
- I see that the numbers in transformRegistry.py have been transplanted from HSC’s distEst, which is fair enough. But I wonder if future generations might thank you for including a bit more information as to their provenance? I see the commit on HSC assures us they were generated from real data, but is that data accessible anywhere? How could I regenerate them if I thought they were faulty in some way? (Maybe these answers are already lost in the mists of time, but if so even noting that might be relevant).
- There are a few other magic defaults in transformRegistry.py — ccdToSkyOrder=9, tolerance=5e-3 etc. Why these values? Maybe a comment to explain?
- This may be due to my misunderstanding of how things are supposed to work, but: if I run bin/genCameraRepository.py hsc/hsc_geom.paf hsc hsc_test and compare the results in hsc_test with those this commit adds to hsc/camera, they are not the same. Is that expected? If there’s some extra magic I need to do to generate the repository, it would be good to document somewhere.
- It’s probably outside the scope of this review since you are updating rather than adding files, but: assuming I am correct and that the files in hsc/camera should be simply the output of bin/genCameraRepository.py, why are they being committed to the repository rather than generated at build time?
- b6aaa0
- I have the same concern here as above: if these files can be generated by the contents of the repository, what are we gaining by versioning them directly?
I hope you'll consider the above (and maybe answer some of my questions!), but I don't feel the need to look a this again before it's merged. However, I'm leaving it in review in case Lauren MacArthur wants to add anything.
On GitHub, Tim Jenness comments:
Can we mark FITS files as binary files using gitattributes so Github doesn't try to show us the diff.
If it really is necessary to version these files (see my comments above, but I'm certainly prepared to believe that it is), then I definitely second Tim's suggestion.
I was thinking of suggesting that we declare FITS files to be binaries in all our repos. In theory we could get clever with a diff tool (by calling fitsheader) but then we'd have to ensure that that command (or the wcsLib version) was always available.
- 59f92b: added explanation to commit message
- a2780b: we added some code on the HSC side to support using a postgresql database for the registry. I don't know if we use it, but I think that given the coming changes to the butler, we should only worry about this when the time comes to pull things back to HSC.
- 030278: added explanation to commit message
- 555767: fixed
- 111d2e:
- Aha, I discovered an off-by-one bug in my reckoning of line lengths in emacs.
- Changed the pickle to actual code.
- I think the test does demonstrate the functionality as it's intended to be used. I don't think we're supposed to instantiate HscDistortion (or other distortion classes) explicitly.
- Added some notes on the provenance of values in transformRegistry.py.
- Added the files from re-running genCameraRepository.py. Suspect an old version got in there somehow.
- I don't think we want to recreate the camera files (created by genCameraRepository.py) at build time, as this is done using "policy" files and an old package (pex_policy) that we expect will be completely removed soon.
Cleaning up, will add something to mark FITS files as binary, and then hope to merge later this afternoon.
Here's where we stand. I plan to merge this in about an hour.
price@price-laptop:~/LSST/obs/subaru (tickets/DM-1794=) $ git sub
|
commit a59f6e69b6546cafa30d48e8f7b6778c48453a31
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 19:53:55 2015 -0400
|
|
genDefectFits.py: fix instantiation of mapper
|
|
We need a root directory, and SUPRIME_DATA_DIR may not be set.
|
It doesn't really matter what root directory we use, because
|
all we really need is the camera.
|
|
bin/genDefectFits.py | 2 +-
|
1 file changed, 1 insertion(+), 1 deletion(-)
|
|
commit a9604db5103714b4621673ee19e7b8441c2b0f40
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 19:54:32 2015 -0400
|
|
registryInfo.py: don't require postgresql
|
|
bin/registryInfo.py | 3 ++-
|
1 file changed, 2 insertions(+), 1 deletion(-)
|
|
commit 141a34cf8d05404aff12626497aebd7f7775cc60
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 19:56:06 2015 -0400
|
|
require SWIGed module before building defects registry
|
|
This fixes race conditions in parallel builds.
|
|
hsc/SConscript | 4 +++-
|
1 file changed, 3 insertions(+), 1 deletion(-)
|
|
commit 47038cc6caa09e721da545e04319c96f9913eb9b
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Tue Aug 4 15:19:25 2015 -0400
|
|
Add .gitattributes to treat FITS files as binary
|
|
.gitattributes | 1 +
|
1 file changed, 1 insertion(+)
|
|
commit 9dab8501770c9bb647f49c8dfb27c4d97d37be54
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:03:30 2015 -0400
|
|
genCameraRepository.py: convert to a plate scale of 1
|
|
LSST likes to use mm/pixels, but Subaru works in nominal pixels from the
|
boresite, or a plate scale of 1. This is true for both Suprime-Cam and
|
HSC.
|
|
This does not include updates to the camera definition files, just the
|
conversion.
|
|
bin/genCameraRepository.py | 6 ++----
|
1 file changed, 2 insertions(+), 4 deletions(-)
|
|
commit 7b9d29b059bbcf002e829cfba699898479d5a458
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:07:46 2015 -0400
|
|
fix amp names
|
|
Subaru amplifiers aren't indexed in two dimensions, so no need for an
|
extra number that's always zero.
|
|
bin/genCameraRepository.py | 2 +-
|
python/lsst/obs/subaru/isr.py | 5 +----
|
2 files changed, 2 insertions(+), 5 deletions(-)
|
|
commit 69fc0b11ae53e480c10147b12ace1bd915826529
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:15:35 2015 -0400
|
|
add HSC distortion model rather than using external package distEst
|
|
This implements distEst using the new cameraGeom framework, and activates
|
the new distortion model.
|
|
bin/genCameraRepository.py | 5 +-
|
hsc/camera/0_00.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_01.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_02.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_03.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_04.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_05.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_06.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_07.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_08.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_09.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_10.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_11.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_12.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_13.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_14.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_15.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_16.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_17.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_18.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_19.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_20.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_21.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_22.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_23.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_24.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_25.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_26.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_27.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_28.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_29.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_30.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_31.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_32.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_33.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_34.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_35.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_36.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_37.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_38.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_39.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_40.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_41.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_42.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_43.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_44.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_45.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_46.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_47.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_48.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_51.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_52.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_53.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_54.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_55.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_56.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/0_57.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_00.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_01.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_02.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_03.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_04.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_05.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_06.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_07.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_08.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_09.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_10.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_11.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_12.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_13.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_14.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_15.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_16.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_17.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_18.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_19.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_20.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_21.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_22.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_23.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_24.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_25.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_26.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_27.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_28.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_29.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_30.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_31.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_32.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_33.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_34.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_35.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_36.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_37.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_38.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_39.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_40.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_41.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_42.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_43.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_44.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_45.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_46.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_47.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_48.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_51.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_52.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_53.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_54.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_55.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_56.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/1_57.fits | Bin 17280 -> 17280 bytes
|
hsc/camera/camera.py | 940 +++++++++++++++++++--------------------
|
include/lsst/obs/subaru/DistEstXYTransform.h | 62 ---
|
include/lsst/obs/subaru/HscDistortion.h | 91 ++++
|
python/lsst/obs/hsc/transformRegistry.py | 281 +++++++++++-
|
python/lsst/obs/subaru/subaruLib.i | 6 +-
|
src/DistEstXYTransform.cc | 63 ---
|
src/HscDistortion.cc | 139 ++++++
|
tests/hscDistortion.py | 1238 ++++++++++++++++++++++++++++++++++++++++++++++++++++
|
ups/obs_subaru.cfg | 11 +-
|
ups/obs_subaru.table | 1 -
|
123 files changed, 2210 insertions(+), 627 deletions(-)
|
|
commit 8c0352f2c1ea86857db37d400cee15a89b79d3ab
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:16:32 2015 -0400
|
|
generate new camera definitions for Suprime-Cam
|
|
suprimecam/camera/Chihiro.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Clarisse.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Fio.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Kiki.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Nausicaa.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Ponyo.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/San.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Satsuki.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Sheeta.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/Sophie.fits | Bin 17280 -> 17280 bytes
|
suprimecam/camera/camera.py | 88 ++++++++++++++++++++++++++++++--------------------------------
|
suprimecam/mit_camera/camera.py | 90 +++++++++++++++++++++++++++++++---------------------------------
|
suprimecam/mit_camera/si001s.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/si002s.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/si005s.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/si006s.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/w4c5.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/w67c1.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/w6c1.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/w7c3.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/w93c2.fits | Bin 17280 -> 17280 bytes
|
suprimecam/mit_camera/w9c2.fits | Bin 17280 -> 17280 bytes
|
22 files changed, 85 insertions(+), 93 deletions(-)
|
|
commit 7259c05d09790420c90071e763ec1bd0137d1c7f
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:16:58 2015 -0400
|
|
cleanup lib/Sconscript
|
|
lib/SConscript | 2 --
|
1 file changed, 2 deletions(-)
|
|
commit d973d1b8595e82e4e2770d327e50fe3b3fb81fb2
|
Author: Paul Price <price@astro.princeton.edu>
|
Date: Thu Jul 23 20:18:03 2015 -0400
|
|
add image of CCD positions for HSC
|
|
This is useful for debugging.
|
|
Taken from http://subarutelescope.org/Observing/Instruments/HSC/CCDPosition_20140811.png
|
|
.gitattributes | 1 +
|
hsc/CCDPosition_20140811.png | Bin 0 -> 370956 bytes
|
2 files changed, 1 insertion(+)
|
Thanks for the clarifications and explanations!
Merged to master. Hooray!
Being related to
DM-245, this issue should therefore be understood as being part of the same Epic.