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

pipe_tasks test_processCcd PSF shape changes with eigen 3.4.0

    XMLWordPrintable

    Details

    • Urgent?:
      No

      Description

      With eigen 3.4.0 installed in the conda environment, the pipe_tasks test_processCcd test fails with an assertion about psfIxx:

      AssertionError: 2.8433291863214247 != 2.843329671276296 within 7 places (4.849548713714569e-07 difference) : psfIxx
      

      This occurs whether jointcal and kht (which should not be used in this test) are built with eigen 3.3.7 or 3.4.0.

      Is the test too strict, or is something else wrong?

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            This is a gen2 test so it's going to be interesting to see whether the test failure manages to survive the proposed move to pipelines_check.

            Show
            tjenness Tim Jenness added a comment - This is a gen2 test so it's going to be interesting to see whether the test failure manages to survive the proposed move to pipelines_check.
            Hide
            yusra Yusra AlSayyad added a comment - - edited

            Joshua Meyers,  I see that in https://github.com/lsst/pipe_tasks/commit/2e9d293356ff2526ce44f1eae8d22c44d82519cd you modified

            2.8433291863214247 to 2.86673377350734 which is a much much greater difference than the the 4e-7 difference that we're keeping eups eigen for.

            Can we conclude from this that the answer is  "the test is too strict"?

             

             

            Show
            yusra Yusra AlSayyad added a comment - - edited Joshua Meyers ,  I see that in https://github.com/lsst/pipe_tasks/commit/2e9d293356ff2526ce44f1eae8d22c44d82519cd  you modified 2.8433291863214247 to 2.86673377350734 which is a much much greater difference than the the 4e-7 difference that we're keeping eups eigen for. Can we conclude from this that the answer is  "the test is too strict"?    
            Hide
            erykoff Eli Rykoff added a comment -

            I think these are two separate issues. One is what happens when we change the algorithm, and the other is what happens when we change the libraries with the same algorithm.

            That said, (a) I do wonder if this test was too strict before; (b) I even more wonder what will happen if we try the update now, if it was a psfex/eigen interaction that was failing (slightly).

            Show
            erykoff Eli Rykoff added a comment - I think these are two separate issues. One is what happens when we change the algorithm, and the other is what happens when we change the libraries with the same algorithm. That said, (a) I do wonder if this test was too strict before; (b) I even more wonder what will happen if we try the update now, if it was a psfex/eigen interaction that was failing (slightly).
            Hide
            wittgen Matthias Wittgen added a comment -

            I need to do a proper test by removing eups Eigen, but updating EUPS and conda eigen to 3.4.0 passes the lsstsw build.

            Show
            wittgen Matthias Wittgen added a comment - I need to do a proper test by removing eups Eigen, but updating EUPS and conda eigen to 3.4.0 passes the lsstsw build.
            Hide
            tjenness Tim Jenness added a comment -

            Looks like you only need to remove eigen from the table files of jointcal and kht.

            Show
            tjenness Tim Jenness added a comment - Looks like you only need to remove eigen from the table files of jointcal and kht.
            Hide
            erykoff Eli Rykoff added a comment -

            Note that the tests have now been moved to pipelines_check.

            Show
            erykoff Eli Rykoff added a comment - Note that the tests have now been moved to pipelines_check .
            Hide
            jmeyers314 Joshua Meyers added a comment -

            > but updating EUPS and conda eigen to 3.4.0 passes the lsstsw build.

            Means there's no longer a problem? 

            While I'm surprised an eigen bump could change this by 4e-7, I don't think we actually need this to be that accurate.

            Show
            jmeyers314 Joshua Meyers added a comment - > but updating EUPS and conda eigen to 3.4.0 passes the lsstsw build. Means there's no longer a problem?  While I'm surprised an eigen bump could change this by 4e-7, I don't think we actually need  this to be that accurate.
            Hide
            wittgen Matthias Wittgen added a comment -

            DM-33496 also relaxed some of the checks.
            There are two PRs now for this ticket. Merging requires a new rubinenv with unpinned eigen being deployed.

            Show
            wittgen Matthias Wittgen added a comment - DM-33496 also relaxed some of the checks. There are two PRs now for this ticket. Merging requires a new rubinenv with unpinned eigen being deployed.
            Hide
            tjenness Tim Jenness added a comment -

            Can we also update the eups package to 3.4.0 so we know that the transition from eups to rubin-env is seamless?

            Show
            tjenness Tim Jenness added a comment - Can we also update the eups package to 3.4.0 so we know that the transition from eups to rubin-env is seamless?
            Hide
            erykoff Eli Rykoff added a comment -

            Or do we need to remove the eups package?

            Show
            erykoff Eli Rykoff added a comment - Or do we need to remove the eups package?
            Hide
            tjenness Tim Jenness added a comment -

            We can remove the EUPS package if the version of eigen in the rubin-env is new enough for our needs. Currently it seems to be pinned at 3.3.9.

            Show
            tjenness Tim Jenness added a comment - We can remove the EUPS package if the version of eigen in the rubin-env is new enough for our needs. Currently it seems to be pinned at 3.3.9.
            Hide
            tjenness Tim Jenness added a comment -

            The EUPS package still has the cholmod patch in it. Did that get applied to 3.4.0 or 3.3.9? I assume we were waiting on 3.4.0 for this reason. It would therefore make sense to update the eigen eups package to 3.4.0 and remove the patch from that package. Then we know we are using 3.4.0 just fine and the two table file removals on this ticket can happen as soon as the rubin-env is updated to 3.4.0.

            Show
            tjenness Tim Jenness added a comment - The EUPS package still has the cholmod patch in it. Did that get applied to 3.4.0 or 3.3.9? I assume we were waiting on 3.4.0 for this reason. It would therefore make sense to update the eigen eups package to 3.4.0 and remove the patch from that package. Then we know we are using 3.4.0 just fine and the two table file removals on this ticket can happen as soon as the rubin-env is updated to 3.4.0.
            Hide
            wittgen Matthias Wittgen added a comment - - edited

            eigen 3.4.0 has the cholmod patch included in the release.
            Yesterday I patched eigen 3.3.9 with the eups patch file and diffed the patched file with the file in the 3.4.0 release. They are identical. Next test I partially did yesterday is to move EUPS eigen to
            3.4.0. See https://github.com/lsst/eigen/tree/u/wittgen/eigen340
            Yesterday a lsstsw build on this branch plus a locally patched conda stack with eigen 3.4.0.
            Today I started the branch in a jenkins pipeline
            https://github.com/lsst/eigen/tree/u/wittgen/eigen340
            Note: that the conda eigen version was mostly used to satisfy dependencies from other conda packages and EUPS eigen was mainly used.

            Show
            wittgen Matthias Wittgen added a comment - - edited eigen 3.4.0 has the cholmod patch included in the release. Yesterday I patched eigen 3.3.9 with the eups patch file and diffed the patched file with the file in the 3.4.0 release. They are identical. Next test I partially did yesterday is to move EUPS eigen to 3.4.0 . See https://github.com/lsst/eigen/tree/u/wittgen/eigen340 Yesterday a lsstsw build on this branch plus a locally patched conda stack with eigen 3.4.0 . Today I started the branch in a jenkins pipeline https://github.com/lsst/eigen/tree/u/wittgen/eigen340 Note: that the conda eigen version was mostly used to satisfy dependencies from other conda packages and EUPS eigen was mainly used.
            Hide
            wittgen Matthias Wittgen added a comment - - edited

            Already approved in github by Tim Jenness , formally assigned this ticket for review as well.

            Show
            wittgen Matthias Wittgen added a comment - - edited Already approved in github by Tim Jenness , formally assigned this ticket for review as well.
            Hide
            tjenness Tim Jenness added a comment -

            Looks fine to me. Please create a new ticket for doing the rubin-env change and make sure this ticket is blocked by that ticket.

            I'm fine with you merging eups eigen 3.4.0 as part of this ticket.

            Show
            tjenness Tim Jenness added a comment - Looks fine to me. Please create a new ticket for doing the rubin-env change and make sure this ticket is blocked by that ticket. I'm fine with you merging eups eigen 3.4.0 as part of this ticket.
            Hide
            wittgen Matthias Wittgen added a comment - - edited

            Eigen 3.4.0 will be updated  by  merging DM-34454 and not as part of this ticket. I am reluctant to overload this ticket as  DM-34454 can be tested independently without implementing DM-34453.

            Remaining question is with DM-34454 should be merged after passing CI as this would result in having Eigen 3.3.9 conda stack and 3.4.0 EUPS. But we had a version mismatch before and it didn't seem to matter.

            Show
            wittgen Matthias Wittgen added a comment - - edited Eigen 3.4.0 will be updated  by  merging DM-34454 and not as part of this ticket. I am reluctant to overload this ticket as  DM-34454 can be tested independently without implementing DM-34453 . Remaining question is with DM-34454 should be merged after passing CI as this would result in having Eigen 3.3.9 conda stack and 3.4.0 EUPS. But we had a version mismatch before and it didn't seem to matter.
            Hide
            erykoff Eli Rykoff added a comment -

            Yes, the eups and conda eigen are kept completely separate, so it's not a problem to have both at the same or different versions.

            Show
            erykoff Eli Rykoff added a comment - Yes, the eups and conda eigen are kept completely separate, so it's not a problem to have both at the same or different versions.
            Hide
            wittgen Matthias Wittgen added a comment -
            Show
            wittgen Matthias Wittgen added a comment - This could be merged to retire building Eigen, passes pipelines https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/36944/pipeline/

              People

              Assignee:
              wittgen Matthias Wittgen
              Reporter:
              ktl Kian-Tat Lim
              Reviewers:
              Tim Jenness
              Watchers:
              Eli Rykoff, Joshua Meyers, Kian-Tat Lim, Matthias Wittgen, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.