Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-1920

update shapeHSM wrappers to latest external version

    Details

      Description

      The HSM shape code has undergone many improvements and bug fixes as part of being included in the GalSim package, and we've recently included those in the HSC fork of meas_extensions_shapeHSM (HSC-129, HSC-1093). We should transfer those changes to the LSST side before tackling DM-981 (or at least before finishing it).

      The story point estimate here is for the work already done on the HSC side (with the usual 50% factor for shared work). The transfer to the LSST side should be essentially no effort. To the extent that EVM cares about this, the credit should go to Paul Price, even though I (Jim Bosch) am doing the transfer.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Paul Price, when you implemented this on the HSC side, did you consider just making GalSim a dependency and building against that? I'm not sure I want to merge all the filtered GalSim git history onto the LSST repo, where I think we're trying to keep the git history a little cleaner. If you just didn't do it because you didn't want to create a eups distrib product for GalSim, I think I might go that route, because I think the sims team has already done that for the LSST side.

            Show
            jbosch Jim Bosch added a comment - Paul Price , when you implemented this on the HSC side, did you consider just making GalSim a dependency and building against that? I'm not sure I want to merge all the filtered GalSim git history onto the LSST repo, where I think we're trying to keep the git history a little cleaner. If you just didn't do it because you didn't want to create a eups distrib product for GalSim, I think I might go that route, because I think the sims team has already done that for the LSST side.
            Hide
            price Paul Price added a comment -

            I wanted to be able to make changes to the GalSim code (and I did make some small changes) without having to fork it as a separate product. I guess I could have forked it on github and built off the fork, but I guess I was charmed by the status quo, which was including the HSM code in the product.

            For LSST, I think it makes sense to fork GalSim on the LSST server, cherry-pick the changes I made, and add the files required to build it in the LSST stack.

            Show
            price Paul Price added a comment - I wanted to be able to make changes to the GalSim code (and I did make some small changes) without having to fork it as a separate product. I guess I could have forked it on github and built off the fork, but I guess I was charmed by the status quo, which was including the HSM code in the product. For LSST, I think it makes sense to fork GalSim on the LSST server, cherry-pick the changes I made, and add the files required to build it in the LSST stack.
            Hide
            ktl Kian-Tat Lim added a comment -

            GalSim has already been packaged for LSST by the Sims group. Any changes could be added as LSST-only patches or pushed upstream, unless they'd cause divergence from the Sims usage, in which case there could be an issue.

            Show
            ktl Kian-Tat Lim added a comment - GalSim has already been packaged for LSST by the Sims group. Any changes could be added as LSST-only patches or pushed upstream, unless they'd cause divergence from the Sims usage, in which case there could be an issue.
            Hide
            jbosch Jim Bosch added a comment -

            Perry Gee it looks like this is going to be a bit harder than I expected, and so I think it's unlikely we'll get to this (and hence DM-981) before the end of the sprint.

            Show
            jbosch Jim Bosch added a comment - Perry Gee it looks like this is going to be a bit harder than I expected, and so I think it's unlikely we'll get to this (and hence DM-981 ) before the end of the sprint.
            Hide
            jbosch Jim Bosch added a comment -

            The changes in meas_extensions_shapeHSM are ready for review - this is just a bunch of slightly-modified cherry-picks from the HSC side, with those modifications being:

            • use new LSST-side names for exceptions
            • don't expect GalSim files to be included in the package; instead build against an external GalSim

            As the original changes were already code-reviewed on the HSC side, we don't need to re-review them here, so I'm just asking Paul Price to take a look at:

            • the difference between HSC master and this, to make sure I haven't gotten anything wrong;
            • the new history, to see if there are new opportunities for squashing commits (I've already done this a little for a few that seemed completely obvious).

            Unfortunately, this can't currently be built without some modifications I've made to our GalSim distrib that are waiting on a resolution to whether those should be pushed to GitHub or the old gitolite servers.

            I also have an action to push two of Paul's changes to GalSim code upstream:

            • Replace an assertion with an exception throw, as it appears possible to trigger the exception when running on real data.
            • GalSim should install its own header files (for now, I've patched our build scripts to do this manually).
            Show
            jbosch Jim Bosch added a comment - The changes in meas_extensions_shapeHSM are ready for review - this is just a bunch of slightly-modified cherry-picks from the HSC side, with those modifications being: use new LSST-side names for exceptions don't expect GalSim files to be included in the package; instead build against an external GalSim As the original changes were already code-reviewed on the HSC side, we don't need to re-review them here, so I'm just asking Paul Price to take a look at: the difference between HSC master and this, to make sure I haven't gotten anything wrong; the new history, to see if there are new opportunities for squashing commits (I've already done this a little for a few that seemed completely obvious). Unfortunately, this can't currently be built without some modifications I've made to our GalSim distrib that are waiting on a resolution to whether those should be pushed to GitHub or the old gitolite servers. I also have an action to push two of Paul's changes to GalSim code upstream: Replace an assertion with an exception throw, as it appears possible to trigger the exception when running on real data. GalSim should install its own header files (for now, I've patched our build scripts to do this manually).
            Hide
            jbosch Jim Bosch added a comment -

            I've just tested the new GalSim build script and .cfg via buildbot, and it seems to be working fine. I'm going to go ahead and merge that to master and tag it, so I can do a eups distrib release that will allow the shapeHSM branch to be built and tested.

            Show
            jbosch Jim Bosch added a comment - I've just tested the new GalSim build script and .cfg via buildbot, and it seems to be working fine. I'm going to go ahead and merge that to master and tag it, so I can do a eups distrib release that will allow the shapeHSM branch to be built and tested.
            Hide
            price Paul Price added a comment -

            Looks like you've got everything in from HSC.

            You could drop a bunch of the early commits, from before we switched to GalSim's HSM (2014-10-23) — they record changes to code that we later removed.

            Good to merge once you've demonstrated it's working.

            Show
            price Paul Price added a comment - Looks like you've got everything in from HSC. You could drop a bunch of the early commits, from before we switched to GalSim's HSM (2014-10-23) — they record changes to code that we later removed. Good to merge once you've demonstrated it's working.
            Hide
            jbosch Jim Bosch added a comment -

            Merged to master.

            I did not delete any of the earlier commits, as I felt it was useful to keep the more recent commits a little more similar to the HSC ones - without the older ones, the changesets look very different in some cases.

            I also haven't waited for the GalSim eups distrib release - I'm basically blocked by lack of permissions and SQuaRE availability on that. There is a GalSim version on lsst-dev (1.2.lsst1) that can be used to build against, so at present this package can only built there (or with a bit of manual work on GalSim). Since this package was bitrotted and failing tests when I found it, I figure it's at least an improvement to be able to build on one system rather than none.

            Show
            jbosch Jim Bosch added a comment - Merged to master. I did not delete any of the earlier commits, as I felt it was useful to keep the more recent commits a little more similar to the HSC ones - without the older ones, the changesets look very different in some cases. I also haven't waited for the GalSim eups distrib release - I'm basically blocked by lack of permissions and SQuaRE availability on that. There is a GalSim version on lsst-dev (1.2.lsst1) that can be used to build against, so at present this package can only built there (or with a bit of manual work on GalSim). Since this package was bitrotted and failing tests when I found it, I figure it's at least an improvement to be able to build on one system rather than none.

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Paul Price
                Watchers:
                Jim Bosch, Kian-Tat Lim, Paul Price
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: