[eigen3] Update to 3.4.0

[spinicist]

Describe the pull request

• What does your PR fix?

  Updates Eigen to 3.4.0

• Which triplets are supported/not supported? Have you updated the [CI baseline]

  all

• Does your PR follow the [maintainer guide]

  Yes

• If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

  Yes

I think this is good to go but have opened it as a draft as it is my first time.

[ghost]

All CLA requirements met.

[autoantwort]

You can download the error logs here. If you hover over the items, there are tree dots on the right that you can click

[spinicist]

You can download the error logs here. If you hover over the items, there are tree dots on the right that you can click

Okay, I took a look at the osx logs and for theia and openmvg these look like genuine regressions to me - they are build failures that reference Eigen. How does the chain of responsibility work here with vcpkg? I've never used those ports so I'm apprehensive about digging about in them.

(An aside: given how low-level Eigen is I'm actually impressed by how few breakages there are here. 3.3 -> 3.4 had some pretty big changes)

[Neumann-A]

How does the chain of responsibility work here with vcpkg?

In generell, for a PR to be merge able CI needs to be green, so all ports which are not green needs to be fixed unless it is a baseline regressions like paraview on osx. (and paraview/vtk works on windows and linux so there is no issue there with eigen3) Sometimes it is just enough to bump the dependent port to a newer version or just report the failure upstream and hopefully they fix it in an timely manner so that the patch can easily be integrated here.

[strega-nil-ms]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[aluaces]

@spinicist, would you mind to update your branch with those requested changes? Otherwise the process is blocked. Thanks!

[NancyLi1013]

@spinicist, would you mind to update your branch with those requested changes? Otherwise the process is blocked. Thanks!

Thanks for your reminder. Since there is no response for a long time. I updated the changes just now.

[spinicist]

Hello,

Thanks for picking this up. I have been on leave and have had a large amount to catch up on.

If I interpret the current build logs correctly, the problems with the regressions in other projects seem to have resolved?

[NancyLi1013]

Hello,

Thanks for picking this up. I have been on leave and have had a large amount to catch up on.

If I interpret the current build logs correctly, the problems with the regressions in other projects seem to have resolved?

Yes, there is nothing else needs to be done now. Just wait for the CI test result. Thanks for your PR @spinicist.

[spinicist]

Looks like those regressions in other projects are still there after all. Should I start reaching out to those projects to see if they have fixes that can be incorporated?

[NancyLi1013]

Except for pangolin, other ports seem to be caused by the update. Could you please look into them?

[NancyLi1013]

The failures caused by pangolinwill be fixed by PR #20006.

[aluaces]

I have taken a look and there are some errors that is not clear is they are eigen's fault or not. For example, I have submitted a question to eigen mailing list in order to know what the solution is for the one affecting openmvg.

Nevertheless, I find a bit difficult to point to the exact failing location of the errors in the automated builds above: if I click on each one I am redirected to an error summary, but it belongs to the whole process and doesn't detail the exact lines and snippets of code failing. Is there a way of seeing that instead of compiling it myself and watching the build logs?

[autoantwort]

You can get the build logs: Click Below on Details -> next Page on View more details on Azure Pipelines -> then on 2 artifacts produced -> search the entry in the list -> hover over it -> click on the tree dots on the right -> click -> download

[aluaces]

I have submitted a fix for the openmvg issue in https://github.com/microsoft/vcpkg/pull/20056

[NancyLi1013]

@aluaces

Could you please resolve the conflicts?

[Wongboo]

@aluaces , looks as if those diffs are not complicated, or may be I can offer help

[aluaces]

@aluaces , looks as if those diffs are not complicated, or may be I can offer help

Yes, I agree. It was just a lack of time, but if you feel like it, be my guest.

EDIT: Hmm, I spoke too soon. You all were referring to the conflicts in this PR, but since I'm not the owner I'm afraid I can do nothing about it, unfortunately.

[Wongboo]

@aluaces , looks like I can't fork a forked repo or try to fetch upstream for the repo without write access (maybe?), where (not how) can I resolve those conflicts?

[aluaces]

@aluaces , looks like I can't fork a forked repo or try to fetch upstream for the repo without write access (maybe?), where (not how) can I resolve those conflicts?

I don't know. I'm in the same boat as you, the PR is not mine so I don't know how to modify it.

[spinicist]

Can I help?

Apologies for going quiet, my day job has been extremely busy.

[ras0219-msft]

I've fixed the merge conflicts and updated the homepage to https; this LGTM once the CI is green.

Thanks for the PR!

[ras0219-msft]

Ok, this update breaks shogun and theia:

Shogun (https://github.com/shogun-toolbox/shogun) has not received a code commit since January and all issues in the last month have been closed as stale (via bot) without any response from a maintainer.

While there isn't recent code activity in Theia (https://github.com/sweeneychris/TheiaSfM), the author posted in May (https://github.com/sweeneychris/TheiaSfM/issues/260#issuecomment-834485676). I've opened an issue https://github.com/sweeneychris/TheiaSfM/issues/261.

If we don't get a response in a week or two I'll prepare this PR to remove both ports from the current tested set (they'll still be accessible via versioning).

At'ing any known users for feedback on this: @cenit, @vigsterkr, @KindDragon

[cenit]

this patch solves the issue with theia (or better, with the akaze third party lib that it includes...). For shogun, i still have to start the work

--- a/libraries/akaze/src/nldiffusion_functions.cpp
+++ b/libraries/akaze/src/nldiffusion_functions.cpp
@@ -222,8 +222,8 @@ void halfsample_image(const RowMatrixXf& src, RowMatrixXf& dst) {

   // Do the whole resampling in one pass by using neighboring values. First, we
   // compute the borders.
-  const double x_kernel_size = static_cast<double>(src.cols()) / dst.cols();
-  const double y_kernel_size = static_cast<double>(src.rows()) / dst.rows();
+  const int x_kernel_size = static_cast<double>(src.cols()) / dst.cols();
+  const int y_kernel_size = static_cast<double>(src.rows()) / dst.rows();

   // Do simple linear interpolation.
   if (x_kernel_size == 2 && y_kernel_size == 2) {
@@ -237,8 +237,8 @@ void halfsample_image(const RowMatrixXf& src, RowMatrixXf& dst) {
     return;
   }

-  const double x_kernel_clamped_size = static_cast<int>(ceil(x_kernel_size));
-  const double y_kernel_clamped_size = static_cast<int>(ceil(y_kernel_size));
+  const int x_kernel_clamped_size = static_cast<int>(ceil(static_cast<double>(src.cols()) / dst.cols()));
+  const int y_kernel_clamped_size = static_cast<int>(ceil(static_cast<double>(src.rows()) / dst.rows()));

   // Set up precomputed factor matrices.
   Eigen::RowVectorXf x_kernel_mul(static_cast<int>(x_kernel_clamped_size)),

[cenit]

upstream open issue with shogun

https://github.com/shogun-toolbox/shogun/issues/4870

and a reference about the problem in the old eigen issue tracker

https://eigen.tuxfamily.org/bz/show_bug.cgi?id=1664

(ported to https://gitlab.com/libeigen/eigen/-/issues/1664 but without any other comments added unfortunately)

[cenit]

let me have some more days to work on shogun, before removing the port...

[spinicist]

(ported to https://gitlab.com/libeigen/eigen/-/issues/1664 but without any other comments added unfortunately)

An FYI that Eigen had problems with their repo at the weekend. Currently no issues that were opened before last weekend have any comments. Hopefully Gitlab will be able to restore them in the near future.

Looking at the Bugzilla thread, it looks like the fix should be straightforward - swapping to the correct .head() or .tail() method instead of .block?

[strega-nil-ms]

waiting for @cenit to figure this out.

[cenit]

(ported to https://gitlab.com/libeigen/eigen/-/issues/1664 but without any other comments added unfortunately)

An FYI that Eigen had problems with their repo at the weekend. Currently no issues that were opened before last weekend have any comments. Hopefully Gitlab will be able to restore them in the near future.

Looking at the Bugzilla thread, it looks like the fix should be straightforward - swapping to the correct .head() or .tail() method instead of .block?

might be. In the meantime, feel free to add the theia patch to fix that port!

[ras0219-msft]

In the theia patch, can we just remove the double casting entirely?

const int y_kernel_size = static_cast<double>(src.rows()) / dst.rows();

becomes

const int y_kernel_size = src.rows() / dst.rows();

[Neumann-A]

In the theia patch, can we just remove the double casting entirely?

I think the idea is to keep it in sync with the clamped calculation where the cast to double is required.

[cenit]

In the theia patch, can we just remove the double casting entirely?

in the first block yes, in the second one no

[cenit]

why is theia completely skipped in CI?

[cenit]

also, there are more regressions that just theia and shogun... here it is a patch to make OpenMVG work

please do not merge eigen 3.4 until all of these ports are fixed!

--- a/src/openMVG/multiview/CMakeLists.txt
+++ b/src/openMVG/multiview/CMakeLists.txt
@@ -38,6 +38,9 @@ target_include_directories(openMVG_multiview
     $<INSTALL_INTERFACE:include>
 )
 set_target_properties(openMVG_multiview PROPERTIES SOVERSION ${OPENMVG_VERSION_MAJOR} VERSION "${OPENMVG_VERSION_MAJOR}.${OPENMVG_VERSION_MINOR}")
+if (MSVC)
+  set_target_properties(openMVG_multiview PROPERTIES COMPILE_FLAGS "/bigobj")
+endif (MSVC)

 add_library(openMVG_multiview_test_data ${MULTIVIEWTESTDATA})
 target_link_libraries(openMVG_multiview_test_data PRIVATE openMVG_numeric openMVG_multiview)

[smancill]

let me have some more days to work on shogun, before removing the port...

This patch should fix shogun 6.1.4

https://github.com/NixOS/nixpkgs/pull/145161/files#diff-ec23d000d3ede3ea11db8129a04b33d1a924523cf2eb52fa247f2a394805971d

[aminya]

Is there an update on this? Eigen 3.4 was released 3 months ago, and it adds necessary patches for critical bugs like this

[spinicist]

Is there an update on this? Eigen 3.4 was released 3 months ago, and it adds necessary patches for critical bugs like this

Apologies - after submitting the original PR my own motivation to get it accepted has declined somewhat, as I'm actually using Eigen 3.4 in vcpkg already (via the branch for this PR).

The issue is there are a couple of other ports that depend on Eigen which do not compile with 3.4. There is a patch to fix one of them above (shogun). I believe after that a similar patch is required for theia, but I know nothing about theia.

[aminya]

How can I do that in my manifest? Should I change my whole vcpkg repository? I wish vcpkg would use fixed versions, so every other package did not break. It is like a loophole. The developers want to try Eigen 3.40, so they can fix their code. But vcpkg does not ship it, because the developers have not fixed their code yet.

[aluaces]

@aminya , you just make a branch with your changes or even clone this branch.

[aminya]

@spinicist Could you please resolve the conflicts so we can see what the state of the CI is? We might move the pull request to someone else's account so that they can continue to fix the issues if you do not have time to fix them.

[ras0219-msft]

How can I do that in my manifest? Should I change my whole vcpkg repository?

You can use overlay ports[1] or registries[2] to replace just eigen with the copy from this PR.

[1] https://github.com/microsoft/vcpkg/blob/master/docs/users/manifests.md#vcpkg_overlay_ports and https://github.com/microsoft/vcpkg/blob/master/docs/specifications/ports-overlay.md [2] https://github.com/microsoft/vcpkg/blob/master/docs/users/registries.md

[LilyWangLL]

Ping @spinicist for response.

[cenit]

here it is also a patch for shogun and eigen 3.4 (note that the openmvg patch i posted above is still not merged in this PR, while the one for theia was merged by @ras0219-msft )

--- a/src/shogun/machine/gp/MultiLaplaceInferenceMethod.cpp
+++ b/src/shogun/machine/gp/MultiLaplaceInferenceMethod.cpp
@@ -84,9 +84,9 @@ class CMultiPsiLine : public func_base
        float64_t result=0;
        for(index_t bl=0; bl<C; bl++)
        {
-           eigen_f.block(bl*n,0,n,1)=K*alpha->block(bl*n,0,n,1)*CMath::exp(log_scale*2.0);
-           result+=alpha->block(bl*n,0,n,1).dot(eigen_f.block(bl*n,0,n,1))/2.0;
-           eigen_f.block(bl*n,0,n,1)+=eigen_m;
+           eigen_f.segment(bl*n,n)=K*alpha->segment(bl*n,n)*CMath::exp(log_scale*2.0);
+           result+=alpha->segment(bl*n,n).dot(eigen_f.segment(bl*n,n))/2.0;
+           eigen_f.segment(bl*n,n)+=eigen_m;
        }

        // get first and second derivatives of log likelihood
@@ -272,7 +272,7 @@ void CMultiLaplaceInferenceMethod::update_alpha()
    {
        Map<VectorXd> alpha(m_alpha.vector, m_alpha.vlen);
        for(index_t bl=0; bl<C; bl++)
-           eigen_mu.block(bl*n,0,n,1)=eigen_ktrtr*CMath::exp(m_log_scale*2.0)*alpha.block(bl*n,0,n,1);
+           eigen_mu.segment(bl*n,n)=eigen_ktrtr*CMath::exp(m_log_scale*2.0)*alpha.segment(bl*n,n);

        //alpha'*(f-m)/2.0
        Psi_New=alpha.dot(eigen_mu)/2.0;
@@ -316,7 +316,7 @@ void CMultiLaplaceInferenceMethod::update_alpha()

        for(index_t bl=0; bl<C; bl++)
        {
-           VectorXd eigen_sD=eigen_dpi.block(bl*n,0,n,1).cwiseSqrt();
+           VectorXd eigen_sD=eigen_dpi.segment(bl*n,n).cwiseSqrt();
            LLT<MatrixXd> chol_tmp((eigen_sD*eigen_sD.transpose()).cwiseProduct(eigen_ktrtr*CMath::exp(m_log_scale*2.0))+
                MatrixXd::Identity(m_ktrtr.num_rows, m_ktrtr.num_cols));
            MatrixXd eigen_L_tmp=chol_tmp.matrixU();
@@ -341,11 +341,11 @@ void CMultiLaplaceInferenceMethod::update_alpha()
        VectorXd tmp2=m_tmp.array().rowwise().sum();

        for(index_t bl=0; bl<C; bl++)
-           eigen_b.block(bl*n,0,n,1)+=eigen_dpi.block(bl*n,0,n,1).cwiseProduct(eigen_mu.block(bl*n,0,n,1)-eigen_mean_bl-tmp2);
+           eigen_b.segment(bl*n,n)+=eigen_dpi.segment(bl*n,n).cwiseProduct(eigen_mu.segment(bl*n,n)-eigen_mean_bl-tmp2);

        Map<VectorXd> &eigen_c=eigen_W;
        for(index_t bl=0; bl<C; bl++)
-           eigen_c.block(bl*n,0,n,1)=eigen_E.block(0,bl*n,n,n)*(eigen_ktrtr*CMath::exp(m_log_scale*2.0)*eigen_b.block(bl*n,0,n,1));
+           eigen_c.segment(bl*n,n)=eigen_E.block(0,bl*n,n,n)*(eigen_ktrtr*CMath::exp(m_log_scale*2.0)*eigen_b.segment(bl*n,n));

        Map<MatrixXd> c_tmp(eigen_c.data(),n,C);

@@ -409,7 +409,7 @@ float64_t CMultiLaplaceInferenceMethod::get_derivative_helper(SGMatrix<float64_t
    {
        result+=((eigen_E.block(0,bl*n,n,n)-eigen_U.block(0,bl*n,n,n).transpose()*eigen_U.block(0,bl*n,n,n)).array()
            *eigen_dK.array()).sum();
-       result-=(eigen_dK*eigen_alpha.block(bl*n,0,n,1)).dot(eigen_alpha.block(bl*n,0,n,1));
+       result-=(eigen_dK*eigen_alpha.segment(bl*n,n)).dot(eigen_alpha.segment(bl*n,n));
    }

    return result/2.0;
@@ -489,7 +489,7 @@ SGVector<float64_t> CMultiLaplaceInferenceMethod::get_derivative_wrt_mean(
        result[i]=0;
        //currently only compute the explicit term
        for(index_t bl=0; bl<C; bl++)
-           result[i]-=eigen_alpha.block(bl*n,0,n,1).dot(eigen_dmu);
+           result[i]-=eigen_alpha.segment(bl*n,n).dot(eigen_dmu);
    }

    return result;

[cenit]

@ras0219-msft since you have power to push here, can you merge with master and add my patches, so that we can see CI status at the end?

[spinicist]

I have some time this week.

I have rebased from master and squashed the commits relating solely to Eigen. Apologies if this was the wrong thing to do.

@cenit If you fork my fork of vcpkg, do you want to add your patches as a PR to the eigen3.4, so that you get credit for them in the history?

[cenit]

i could do that but i might not have time this week. if you have time please go on and apply them

[spinicist]

Okay, fingers crossed I have incorporated the patches from @cenit correctly.

@LilyWangLL the Github actions bot still shows requests that I think have been met now?

[cenit]

you forgot to bump port-version after recent patches

[cenit]

after that, you will still need a manual approval for ci tests since you are a first time contributor