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

    • 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

            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.

            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.
            yusra Yusra AlSayyad added a comment - - edited

            jmeyers314,  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"?

             

             

            yusra Yusra AlSayyad added a comment - - edited jmeyers314 ,  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"?    
            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).

            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).

            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.

            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.
            tjenness Tim Jenness added a comment -

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

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

            Note that the tests have now been moved to pipelines_check.

            erykoff Eli Rykoff added a comment - Note that the tests have now been moved to pipelines_check .

            > 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.

            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.

            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.

            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.
            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?

            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?
            erykoff Eli Rykoff added a comment -

            Or do we need to remove the eups package?

            erykoff Eli Rykoff added a comment - Or do we need to remove the eups package?
            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.

            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.
            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.

            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.
            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.

            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.
            wittgen Matthias Wittgen added a comment - - edited

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

            wittgen Matthias Wittgen added a comment - - edited Already approved in github by tjenness , formally assigned this ticket for review as well.
            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.

            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.
            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.

            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.
            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.

            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.
            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

              wittgen Matthias Wittgen
              ktl Kian-Tat Lim
              Tim Jenness
              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.