Eigen 3.4 compatibility

[jdtournier]

Remove Eigen::Vectors/4 typedefs since these are defined in Eigen 3.4 and cause conflicts. Instead, just use the standard Eigen::Vector3d typedef, which is identical currently. This may cause issues if we ever decide to revert the default_type typedef from double (its current value) to float or something, but I don't see that happening any time soon. Besides, even if we do change default_type, there's a good argument that this shouldn't influence the vast majority of these instance of Vector3, since this is likely to break all kinds of operations.

These changes are required to pass CI on macOS, since homebrew now ships with Eigen 3.4.

[jdtournier]

Looks like Eigen 3.4 introduces subtle differences in precision. Actually not so subtle in the case of dwidenoise, but that only happens with the Exp1 estimator. Not sure what to make of that...

For now, I reckon we pin the Eigen version to 3.3 in homebrew for the macOS CI - that'll at least allow the CI to complete, we can investigate the issues with precision at a later time.

[jdtournier]

@maxpietsch: any idea how to request eigen version 3.3.X in homebrew...?

[maxpietsch]

I don't think it's supposed to be possible in homebrew to get legacy versions of packages. There seems to be a workaround using the old formula but maybe just clone eigen from https://gitlab.com/libeigen/eigen/-/tree/3.3?

Edit:

Actually, the raw formula trick does not work anymore, results in Invalid usage: Installation of eigen from a GitHub commit URL is unsupported! brew extract eigen to a stable tap on GitHub instead. (UsageError). Also this does not work directly with core packages but one can create a local tap (from https://stackoverflow.com/a/64125796/2389450):

# brew unlink eigen
brew tap-new core/eigen
brew extract eigen --version 3.3.9 core/eigen
brew install core/eigen/eigen@3.3.9

[Lestropie]

This may cause issues if we ever decide to revert the default_type typedef from double (its current value) to float or something, but I don't see that happening any time soon. Besides, even if we do change default_type, there's a good argument that this shouldn't influence the vast majority of these instance of Vector3, since this is likely to break all kinds of operations.

Question is whether or not we should explicitly test whether or not default_type and Eigen::Vector3d::Scalar are equivalent. That is to say, if they were to diverge, is there a chance of silent regression, whether from compilers performing implicit conversion without warning, or from a lack of CI coverage.

[jdtournier]

This may cause issues if we ever decide to revert the default_type typedef from double (its current value) to float or something, but I don't see that happening any time soon. Besides, even if we do change default_type, there's a good argument that this shouldn't influence the vast majority of these instance of Vector3, since this is likely to break all kinds of operations.

Question is whether or not we should explicitly test whether or not default_type and Eigen::Vector3d::Scalar are equivalent. That is to say, if they were to diverge, is there a chance of silent regression, whether from compilers performing implicit conversion without warning, or from a lack of CI coverage.

Well, Eigen's Vector3d is very explicitly a vector of type double, I can't imagine they would ever change that silently. So if there's any divergence, it would be down to us changing our own default_type, and I reckon we wouldn't do that lightly...

[jdtournier]

@maxpietsch,

one can create a local tap

So can we add that to the CI workflow for macOS?

[maxpietsch]

Tests also failed with eigen 3.3.9, trying 3.3.7 as in eigen 3.3.8, SelfAdjointEigenSolver which is used in dwidenoise and tensor2metric average_space and deterministic tensor tracking was changed to address precision issues: https://gitlab.com/libeigen/eigen/-/commit/0dd9643ad547d3dd2e23ded1d3376d0f7bdc8ada

[jdtournier]

Good find @maxpietsch! Question then is whether the later versions actually provide more accurate results, in which case maybe we should update the test data to match...?

This would then mean that testing against older versions of Eigen would fail... Not sure how best to deal with this.

[Lestropie]

This would then mean that testing against older versions of Eigen would fail... Not sure how best to deal with this.

Save pre-calculated results using both versions; flag as error if test output matches neither.
I used this somewhere to deal with FSL 5 vs. 6 bet differences, because in 5ttgen the image cropping is done based on the brain mask so the image dimensions come out differently; but not sure where the code is, looks like it hasn't made it to master.

[jdtournier]

Save pre-calculated results using both versions; flag as error if test output matches neither.

Yes, that's more or less what was going through my mind too... I'll need to spend a bit of time figuring this out. In which case there would be no point in tweaking the CI tests, @maxpietsch, let's leave them as-is for now.

[jdtournier]

I had a look into the test failures with Eigen 3.4, and they're really due to quite minor differences in precision. For dwidenoise with the Exp1 estimator, the tests pass OK if I reduce the absolute precision to 1e-3 - which equates to a relative precision of around 1e-5 given that the mean DWI values in there are around the 200-300 mark, so I reckon that's actually a fair level of tolerance. I don't think we need to duplicate the test data at this point. Is that OK with everyone?

[bjeurissen]

Looks good!

[jdtournier]

OK, I'll merge this to master as it is, and then merge master back to dev. When I do, I'll update the test data with results using Eigen 3.4. Rationale being to avoid making unnecessary changes to master that might look more significant than they really are (which modifying the test data might suggest...).