Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-479

Upgrade Eigen to 3.3.4

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      Our current version of Eigen is 3.2.5, which does not support CholmodBase.determinant(), which I would like to have for debugging jointcal. I suspect it will bring in a variety of other updates we will want to use in jointcal and afw.

      I fear that this won't be a trivial upgrade: there have been quite a few releases between 3.2.5 and 3.3.4:

      http://eigen.tuxfamily.org/index.php?title=ChangeLog#Eigen_3.3-alpha1

      My first naive attempt to build and use a new Eigen with jointcal resulted in a slew of ndarray errors, so there's going to be some C++/ndarray upgrade work to make this happen.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment - - edited

            This is worth doing (we don't want to be stuck on an old Eigen), but I worry that it will be sufficiently difficult that we should find some other way to get that determinant calculation.  Here's the full story (copying and pasting from a Slack conversation with John Parejko):

            • The first bit of bad news is that ndarray::EigenView has always been built on top of Eigen interfaces that were not documented and not considered to be public by the Eigen team, so it's not shocking that they've changed underneath us.  But using those private interfaces was and continues to be the only way to write something like ndarray::EigenView: it's an object with Eigen-compatible operators but ndarray/numpy-compatible reference-counted memory management.
            • The good news is that we may not need ndarray::EigenView anymore; its primary purpose was to allow ndarray to convert Eigen objects to Python, and to manage that memory safely on the C++ side.  But pybind11 now has its own bindings for Eigen that are probably as good as ndarray's, though they have a different philosophy for memory management.
            • The next piece of bad news is that we would need to convert all of our code at once to use pybind11's Eigen bindings; a package-by-package change would not work, because the two approaches define the same template specialization different ways, and using them both in shared libraries that can see each other's symbols would be a violation of the One Definition Rule.  Building our pybind11 wrappers with symbols hidden by default would probably allow us to sidestep that issue, and is probably worth doing for its own sake (it may, for instance, help improve build times, build sizes, and module import times), but it will probably also take some effort to get working.
            • The last piece of bad news is that we do occasionally use  ndarray::EigenView in regular C++ code, especially in the bowels of the shapelet and meas_modelfit packages, and if we wanted to avoid continuing to rely on private Eigen interfaces, we'd need to modify that code to be able to work on regular Eigen::Map objects.  That would require a lot of care and probably some refactoring, because it isn't a drop-in replacement; Eigen::Map doesn't manage its own memory, and can't be used safely (e.g.) as a data member without a lot of care.

            So, the hopefully simple way to upgrade Eigen is to keep ndarray::EigenView and just fix whatever broke; "hopeful" because I don't know what changed in the private interfaces it relies on, and there's a chance that fixing it will essentially require reimplementing it on a new set of private Eigen interfaces.  The more principled fix is to drop it, but that's both a lot of effort overall and the kind of effort that may require a lot of input from me, since I'm the big offender in terms of ndarray::EigenView usage in the current codebase (mea culpa).

             

            Show
            jbosch Jim Bosch added a comment - - edited This is worth doing (we don't want to be stuck on an old Eigen), but I worry that it will be sufficiently difficult that we should find some other way to get that determinant calculation.  Here's the full story (copying and pasting from a Slack conversation with John Parejko ): The first bit of bad news is that ndarray::EigenView has always been built on top of Eigen interfaces that were not documented and not considered to be public by the Eigen team, so it's not shocking that they've changed underneath us.  But using those private interfaces was and continues to be the only way to write something like ndarray::EigenView : it's an object with Eigen-compatible operators but ndarray/numpy-compatible reference-counted memory management. The good news is that we may not need ndarray::EigenView  anymore; its primary purpose was to allow ndarray to convert Eigen objects to Python, and to manage that memory safely on the C++ side.  But pybind11 now has its own bindings for Eigen that are probably as good as ndarray's, though they have a different philosophy for memory management. The next piece of bad news is that we would need to convert all of our code at once to use pybind11's Eigen bindings; a package-by-package change would not work, because the two approaches define the same template specialization different ways, and using them both in shared libraries that can see each other's symbols would be a violation of the One Definition Rule.  Building our pybind11 wrappers with symbols hidden by default would probably allow us to sidestep that issue, and is probably worth doing for its own sake (it may, for instance, help improve build times, build sizes, and module import times), but it will probably also take some effort to get working. The last piece of bad news is that we do occasionally use  ndarray::EigenView  in regular C++ code, especially in the bowels of the shapelet and meas_modelfit packages, and if we wanted to avoid continuing to rely on private Eigen interfaces, we'd need to modify that code to be able to work on regular Eigen::Map  objects.  That would require a lot of care and probably some refactoring, because it isn't a drop-in replacement; Eigen::Map doesn't manage its own memory, and can't be used safely (e.g.) as a data member without a lot of care. So, the hopefully simple way to upgrade Eigen is to keep ndarray::EigenView and just fix whatever broke; "hopeful" because I don't know what changed in the private interfaces it relies on, and there's a chance that fixing it will essentially require reimplementing it on a new set of private Eigen interfaces.  The more principled fix is to drop it, but that's both a lot of effort overall and the kind of effort that may require a lot of input from me, since I'm the big offender in terms of ndarray::EigenView usage in the current codebase (mea culpa).  
            Hide
            rowen Russell Owen added a comment - - edited

            Jim Bosch regarding your last point (using ndarray::EigenView in C+): can that be changed independent of how we deal with our pybind11 wrappers (e.g. could the C+ code be rewritten to use documented Eigen interfaces)? To my mind that sounds worth doing regardless of how we proceed with pybind11.

            Show
            rowen Russell Owen added a comment - - edited Jim Bosch regarding your last point (using ndarray::EigenView in C+ ) : can that be changed independent of how we deal with our pybind11 wrappers (e.g. could the C+ code be rewritten to use documented Eigen interfaces)? To my mind that sounds worth doing regardless of how we proceed with pybind11.
            Hide
            jbosch Jim Bosch added a comment -

            Yes, we could rewrite the C++ code using documented Eigen interfaces without converting our pybind11 usage - we just won't be able to remove ndarray::EigenView until we've done both, and we'd first want to add some new utility code to ndarray that is based on those public interfaces, rather than distributing copies of the "make Eigen::Map from ndarray::Array" snippet throughout our codebase.

            Show
            jbosch Jim Bosch added a comment - Yes, we could rewrite the C++ code using documented Eigen interfaces without converting our pybind11 usage - we just won't be able to remove ndarray::EigenView until we've done both, and we'd first want to add some new utility code to ndarray that is based on those public interfaces, rather than distributing copies of the "make Eigen::Map from ndarray::Array" snippet throughout our codebase.
            Hide
            tjenness Tim Jenness added a comment -

            John Parejko do you want to extend the date for this RFC or adopt it on the understanding that in theory we want to do this but we might have to pause if it becomes more difficult than we want to handle?

            Show
            tjenness Tim Jenness added a comment - John Parejko do you want to extend the date for this RFC or adopt it on the understanding that in theory we want to do this but we might have to pause if it becomes more difficult than we want to handle?
            Hide
            Parejkoj John Parejko added a comment -

            As Tim Jenness says above: given the agreement that we want to upgrade, I'm going to adopt this. We'll see how far we can actually get with the implementation (DM-14305).

            Show
            Parejkoj John Parejko added a comment - As Tim Jenness says above: given the agreement that we want to upgrade, I'm going to adopt this. We'll see how far we can actually get with the implementation ( DM-14305 ).
            Hide
            rowen Russell Owen added a comment - - edited

            For the record: EigenView is used in afw (LeastSquares.cc), ip_diffIm (KernelSolution.cc), meas_astrom (PolynomialTransform.h) and meas_base (PsfFlux.cc), as well as meas_modelfit and shapelet. Of these, I think the only usage is in .cc files except in meas_astrom where EigenView is used for two class member variables.

            Show
            rowen Russell Owen added a comment - - edited For the record: EigenView is used in afw (LeastSquares.cc), ip_diffIm (KernelSolution.cc), meas_astrom (PolynomialTransform.h) and meas_base (PsfFlux.cc), as well as meas_modelfit and shapelet. Of these, I think the only usage is in .cc files except in meas_astrom where EigenView is used for two class member variables.
            Hide
            jbosch Jim Bosch added a comment -

            I think the right approach to evolving code where EigenView is used as a member is to use ndarray::Array as a member instead, make temporary Eigen::Map views when an Eigen interface is needed, and make sure any assignment operators behave the same way (EigenView does deep assignment, Array's is shallow).

            Show
            jbosch Jim Bosch added a comment - I think the right approach to evolving code where EigenView is used as a member is to use ndarray::Array  as a member instead, make temporary Eigen::Map views when an Eigen interface is needed, and make sure any assignment operators behave the same way ( EigenView does deep assignment, Array 's is shallow).

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Paul Price, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End: