# pipe_tasks test_processCcd PSF shape changes with eigen 3.4.0

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• 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?

#### Activity

No builds found.
Kian-Tat Lim created issue -
Field Original Value New Value
 Labels SciencePipelines
Hide
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
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.
 Component/s eigen [ 12881 ]
 Labels SciencePipelines PairCoding SciencePipelines
Hide

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 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"?
 Watchers Eli Rykoff, Kian-Tat Lim, Tim Jenness, Yusra AlSayyad [ Eli Rykoff, Kian-Tat Lim, Tim Jenness, Yusra AlSayyad ] Eli Rykoff, Kian-Tat Lim, Matthias Wittgen, Tim Jenness, Yusra AlSayyad [ Eli Rykoff, Kian-Tat Lim, Matthias Wittgen, Tim Jenness, Yusra AlSayyad ]
Hide
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
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).
 Link This issue relates to DM-33073 [ DM-33073 ]
Hide
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
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.
 Assignee Matthias Wittgen [ wittgen ]
 Link This issue duplicates DM-34439 [ DM-34439 ]
 Link This issue duplicates DM-34439 [ DM-34439 ]
Hide
Tim Jenness added a comment -

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

Show
Tim Jenness added a comment - Looks like you only need to remove eigen from the table files of jointcal and kht.
 Link This issue duplicates DM-33496 [ DM-33496 ]
Hide
Eli Rykoff added a comment -

Note that the tests have now been moved to pipelines_check.

Show
Eli Rykoff added a comment - Note that the tests have now been moved to pipelines_check .
 Status To Do [ 10001 ] In Progress [ 3 ]
Hide
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
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
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
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
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
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
Eli Rykoff added a comment -

Or do we need to remove the eups package?

Show
Eli Rykoff added a comment - Or do we need to remove the eups package?
Hide
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
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
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
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
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
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
Matthias Wittgen added a comment - - edited

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

Show
Matthias Wittgen added a comment - - edited Already approved in github by Tim Jenness , formally assigned this ticket for review as well.
 Reviewers Tim Jenness [ tjenness ] Status In Progress [ 3 ] In Review [ 10004 ]
 Link This issue is blocked by DM-34453 [ DM-34453 ]
Hide
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
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.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
 Link This issue relates to DM-34454 [ DM-34454 ]
Hide
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
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
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
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
Matthias Wittgen added a comment -
Show
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/
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]

#### People

Assignee:
Matthias Wittgen
Reporter:
Kian-Tat Lim
Reviewers:
Tim Jenness
Watchers:
Eli Rykoff, Joshua Meyers, Kian-Tat Lim, Matthias Wittgen, Tim Jenness, Yusra AlSayyad