Skip to content

Conversation

@conradsnicta
Copy link
Contributor

Bump minimum Armadillo version to 10.8.

This allows using the numerous Armadillo API additions and enhancements since 9.800. It also allows assuming throughout the mlpack codebase that matrices are initialised to zero by default.

Current state of Armadillo versions in various distros:

  • Ubuntu 22.04 LTS has Armadillo 10.8.2
  • Ubuntu 24.04 LTS has Armadillo 12.6.7
  • Debian 12 has Armadillo 11.4.2
  • Debian 13 (testing) has Armadillo 12.8.2
  • RHEL EPEL 8 & 9 has Armadillo 12.6.6 (subject to future upgrades)

Related: mlpack/ensmallen#404

@rcurtin
Copy link
Member

rcurtin commented Jul 10, 2024

Looks good; we can also remove these legacy workarounds now:

$ grep -r ARMA_VERSION .
./mlpack/core/util/arma_traits.hpp:#if ((ARMA_VERSION_MAJOR >= 10) || \
./mlpack/core/util/arma_traits.hpp:    ((ARMA_VERSION_MAJOR == 9) && (ARMA_VERSION_MINOR >= 869)))
./mlpack/bindings/go/mlpack/capi/arma_util.hpp:    #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/python/mlpack/arma_util.hpp:  #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/julia/julia_util.cpp:    #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/julia/julia_util.cpp:    #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/julia/julia_util.cpp:    #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/julia/julia_util.cpp:    #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/julia/julia_util.cpp:    #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/julia/julia_util.cpp:    #if ARMA_VERSION_MAJOR >= 10
./mlpack/bindings/julia/julia_util.cpp:    #if ARMA_VERSION_MAJOR >= 10

I think we will have to wait on the ensmallen version to get bumped but that shouldn't take long.

@conradsnicta
Copy link
Contributor Author

@rcurtin legacy workarounds removed

Copy link
Member

@shrit shrit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

shrit
shrit previously requested changes Jul 11, 2024
Copy link
Member

@shrit shrit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting change so we fix the CI before

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, everything looks great. I pushed some simple changes to update the version used in CI; I'll work to get everything green there.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second approval provided automatically after 24 hours. 👍

@rcurtin
Copy link
Member

rcurtin commented Jul 12, 2024

@mlpack-jenkins test this please

@conradsnicta conradsnicta merged commit a1ecfab into master Jul 13, 2024
@conradsnicta conradsnicta deleted the arma-version-bump branch July 13, 2024 07:04
github-actions[bot]

This comment was marked as resolved.

@rcurtin rcurtin mentioned this pull request Sep 16, 2024
@rcurtin rcurtin mentioned this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants