Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update deprecated .max(ind), .min(ind) to .index_max(), .index_min() #3835

Merged
merged 8 commits into from
Nov 24, 2024
Merged

Update deprecated .max(ind), .min(ind) to .index_max(), .index_min() #3835

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a8181b0
Update deprecated .max(ind), .min(ind) to .index_max(), .index_min()
eddelbuettel Nov 19, 2024
a09ae52
Correct use of index_max() result
eddelbuettel Nov 20, 2024
5b490e4
Revert perceptron_impl.hpp
eddelbuettel Nov 20, 2024
22d678e
update to use .index_max()
conradsnicta Nov 22, 2024
db6b014
Remove unused variable
eddelbuettel Nov 22, 2024
8877e2c
Cosmetic whitespace treatment, error only on error
eddelbuettel Nov 22, 2024
bbbaf7e
Another max() -> index_max() change
eddelbuettel Nov 23, 2024
69726bf
Revert rcmdcheck setting to 'error on warning' [ci skip]
eddelbuettel Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ jobs:
shell: Rscript {0}

- name: Check
run: Rscript -e "rcmdcheck::rcmdcheck('${{ needs.jobR.outputs.r_bindings }}', args = c('--no-manual','--as-cran'), error_on = 'warning', check_dir = 'check')"
run: Rscript -e "rcmdcheck::rcmdcheck('${{ needs.jobR.outputs.r_bindings }}', args = c('--no-manual','--as-cran'), error_on = 'error', check_dir = 'check')"
Copy link
Member

Choose a reason for hiding this comment

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

Since CRAN will reject for warnings, should we revert this to warning? You know better than me.

Copy link
Contributor Author

@eddelbuettel eddelbuettel Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. We should revert once we have a new RcppEnsmallen at CRAN. Until then we are guaranteed

  • a warning because of the no-heads-up-given-to-us deprecatation in Armadillo 14.2.0
  • a guaranteed fail when we error on warning

which is why I flipped this for now. You as maintainer are free to overrule but you then have to live with all the ❌ that is entails 🙀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since CRAN will reject for warnings

My approach with my (smaller !!) CRAN packages has been to deal with that locally not force my CIs to mail over what are often transient nuisances. But different strokes for different folks -- you are clearly a much a much more patient person than I am. I dislike it when CI runs take over ten minutes. Several hours, spread over different backend systems is ... a different cup of tea.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah unfortunately with how long mlpack takes to compile in general (as with any heavily template metaprogrammed C++ application...), I don't have any way to get away from long turnaround times for CI. So I'm not a very patient person in general but I have had to learn some of it for our CI infrastructure. (I guess that's probably a good thing for me as a person overall.)

I saw your comment about releasing a new version of ensmallen---I think that's a good idea and I'll probably do it over the weekend, just a bugfix patch or something. That in turn should mean a fairly quick turnaround for a new version of RcppEnsmallen, and then we have no issue here. Personally I would rather do that than disable the error and forget to revert (which honestly I think is fairly high probability).

no-heads-up-given-to-us

Technically the whole thing is my fault 😄 I noticed that the overload was not documented and pointed it out. And honestly I think a deprecation warning is a totally reasonable way to provide the heads-up (in fact that's what deprecation is: a heads-up). My issue is more with CRAN's policy that makes the deprecation warning suddenly unacceptable. (Of course, the good side of that: the deprecated function usage gets resolved very quickly!)

Copy link
Contributor

Choose a reason for hiding this comment

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

sidenote: I've empirically found that using ionice -c 3 in conjunction with standard nice tends to make multi-core compilation of mlpack less taxing; example:

$ ionice -c 3 nice make -j 4 mlpack_test

ionice -c 3 prevents I/O access by a process until the I/O queue is clear: https://booksonline.nl/man/htmlman1/ionice.1.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re 'no heads-up given' and 'technically my fault': I am for deprecation too, but you MUST managed them downstream if someone like CRAN checks. More than once I had a really long open issue tackling every downstream package til a patch was in at CRAN. See eg this one for an example. So in my view the only way we can handle this is to

  • create a deprecation warning as opt-in, leave it off by default
  • turn it, find all affected downstream packages, changes all of them (!!)
  • only once that is done, turn it on by default

Consequently, RcppArmadillo is forced to turn this off, as upsetting as that is for @conradsnicta. It affects too many packages. mlpack got bitten at CRAN because it uses Arma header directly.

Re ionice: Awesome. Will try. I had to give up on make -j 4 so can try this.

ccache also helps as it reduces what gets compiled. I may open an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rcurtin I can of course also revert this change now, given that we get a merge over ❌ anyway. Let me know if you'd prefer that.

And thanks for the review @conradsnicta !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rcurtin I will leave this to you and revert it now as it is not really a core part of the PR, and did not help in "getting to ✔️" which other parts failing.


- name: Upload check results
if: failure()
Expand Down
2 changes: 1 addition & 1 deletion src/mlpack/methods/adaboost/adaboost_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ void AdaBoost<WeakLearnerType, MatType>::Classify(
for (size_t i = 0; i < predictedLabels.n_cols; ++i)
{
probabilities.col(i) /= accu(probabilities.col(i));
probabilities.col(i).max(maxIndex);
maxIndex = probabilities.col(i).index_max();
predictedLabels(i) = maxIndex;
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/mlpack/methods/approx_kfn/drusilla_select_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ void DrusillaSelect<MatType>::Train(
for (size_t i = 0; i < l; ++i)
{
// Pick best index.
arma::uword maxIndex = 0;
norms.max(maxIndex);
arma::uword maxIndex = norms.index_max();

arma::vec line(refCopy.col(maxIndex) / norm(refCopy.col(maxIndex)));

Expand Down
3 changes: 1 addition & 2 deletions src/mlpack/methods/decision_tree/decision_tree_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1159,8 +1159,7 @@ void DecisionTree<FitnessFunction,

// Now normalize into probabilities.
classProbabilities /= UseWeights ? sumWeights : labels.n_elem;
arma::uword maxIndex = 0;
classProbabilities.max(maxIndex);
arma::uword maxIndex = classProbabilities.index_max();
majorityClass = (size_t) maxIndex;
}

Expand Down
5 changes: 3 additions & 2 deletions src/mlpack/methods/hmm/hmm_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,13 +528,14 @@ double HMM<Distribution>::Predict(const arma::mat& dataSeq,
for (size_t j = 0; j < logTransition.n_rows; j++)
{
arma::vec prob = logStateProb.col(t - 1) + logTransition.row(j).t();
logStateProb(j, t) = prob.max(index) + logProbs(t, j);
index = prob.index_max();
logStateProb(j, t) = prob[index] + logProbs(t, j);
stateSeqBack(j, t) = index;
}
}

// Backtrack to find the most probable state sequence.
logStateProb.unsafe_col(dataSeq.n_cols - 1).max(index);
index = logStateProb.unsafe_col(dataSeq.n_cols - 1).index_max();
stateSeq[dataSeq.n_cols - 1] = index;
for (size_t t = 2; t <= dataSeq.n_cols; t++)
{
Expand Down
8 changes: 3 additions & 5 deletions src/mlpack/methods/hoeffding_trees/binary_numeric_split_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,9 @@ void BinaryNumericSplit<FitnessFunction, ObservationType>::Split(
}

// Calculate the majority classes of the children.
arma::uword maxIndex;
counts.unsafe_col(0).max(maxIndex);
arma::uword maxIndex = counts.unsafe_col(0).index_max();
childMajorities[0] = size_t(maxIndex);
counts.unsafe_col(1).max(maxIndex);
maxIndex = counts.unsafe_col(1).index_max();
childMajorities[1] = size_t(maxIndex);

// Create the according SplitInfo object.
Expand All @@ -155,8 +154,7 @@ template<typename FitnessFunction, typename ObservationType>
size_t BinaryNumericSplit<FitnessFunction, ObservationType>::MajorityClass()
const
{
arma::uword maxIndex;
classCounts.max(maxIndex);
arma::uword maxIndex = classCounts.index_max();
return size_t(maxIndex);
}

Expand Down
6 changes: 2 additions & 4 deletions src/mlpack/methods/hoeffding_trees/hoeffding_categorical_split_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ void HoeffdingCategoricalSplit<FitnessFunction>::Split(
childMajorities.set_size(sufficientStatistics.n_cols);
for (size_t i = 0; i < sufficientStatistics.n_cols; ++i)
{
arma::uword maxIndex = 0;
sufficientStatistics.unsafe_col(i).max(maxIndex);
arma::uword maxIndex = sufficientStatistics.unsafe_col(i).index_max();
childMajorities[i] = size_t(maxIndex);
}

Expand All @@ -79,8 +78,7 @@ size_t HoeffdingCategoricalSplit<FitnessFunction>::MajorityClass() const
// Calculate the class that we have seen the most of.
arma::Col<size_t> classCounts = sum(sufficientStatistics, 1);

arma::uword maxIndex = 0;
classCounts.max(maxIndex);
arma::uword maxIndex = classCounts.index_max();

return size_t(maxIndex);
}
Expand Down
9 changes: 3 additions & 6 deletions src/mlpack/methods/hoeffding_trees/hoeffding_numeric_split_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,7 @@ void HoeffdingNumericSplit<FitnessFunction, ObservationType>::Split(
childMajorities.set_size(sufficientStatistics.n_cols);
for (size_t i = 0; i < sufficientStatistics.n_cols; ++i)
{
arma::uword maxIndex = 0;
sufficientStatistics.unsafe_col(i).max(maxIndex);
arma::uword maxIndex = sufficientStatistics.unsafe_col(i).index_max();
childMajorities[i] = size_t(maxIndex);
}

Expand All @@ -144,8 +143,7 @@ size_t HoeffdingNumericSplit<FitnessFunction, ObservationType>::
for (size_t i = 0; i < samplesSeen; ++i)
classes[labels[i]]++;

arma::uword majorityClass;
classes.max(majorityClass);
arma::uword majorityClass = classes.index_max();
return size_t(majorityClass);
}
else
Expand All @@ -154,8 +152,7 @@ size_t HoeffdingNumericSplit<FitnessFunction, ObservationType>::
// statistics.
arma::Col<size_t> classCounts = sum(sufficientStatistics, 1);

arma::uword maxIndex = 0;
classCounts.max(maxIndex);
arma::uword maxIndex = classCounts.index_max();
return size_t(maxIndex);
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/mlpack/methods/kmeans/max_variance_new_cluster_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ void MaxVarianceNewCluster::EmptyCluster(const MatType& data,
this->iteration = iteration;

// Now find the cluster with maximum variance.
arma::uword maxVarCluster = 0;
variances.max(maxVarCluster);
arma::uword maxVarCluster = variances.index_max();

// If the cluster with maximum variance has variance of 0, then we can't
// continue. All the points are the same.
Expand Down
3 changes: 1 addition & 2 deletions src/mlpack/methods/naive_bayes/naive_bayes_classifier_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,7 @@ void NaiveBayesClassifier<ModelMatType>::Classify(
// Now calculate maximum probabilities for each point.
for (size_t i = 0; i < data.n_cols; ++i)
{
arma::uword maxIndex = 0;
logLikelihoods.unsafe_col(i).max(maxIndex);
arma::uword maxIndex = logLikelihoods.unsafe_col(i).index_max();
conradsnicta marked this conversation as resolved.
Show resolved Hide resolved
predictions[i] = maxIndex;
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/mlpack/methods/perceptron/perceptron_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ void Perceptron<
size_t j, i = 0;
bool converged = false;
size_t tempLabel;
arma::uword maxIndexRow = 0, maxIndexCol = 0;
arma::uword maxIndexRow = 0;
arma::Mat<ElemType> tempLabelMat;

LearnPolicy LP;
Expand All @@ -244,7 +244,8 @@ void Perceptron<
// correctly classifies this.
tempLabelMat = weights.t() * data.col(j) + biases;

tempLabelMat.max(maxIndexRow, maxIndexCol);
maxIndexRow = arma::ind2sub(arma::size(tempLabelMat),
tempLabelMat.index_max())(0);

// Check whether prediction is correct.
if (maxIndexRow != labels(0, j))
Expand Down Expand Up @@ -289,7 +290,7 @@ size_t Perceptron<LearnPolicy, WeightInitializationPolicy, MatType>::Classify(
arma::uword maxIndex = 0;

tempLabelVec = weights.t() * point + biases;
tempLabelVec.max(maxIndex);
maxIndex = tempLabelVec.index_max();

return size_t(maxIndex);
}
Expand Down Expand Up @@ -322,7 +323,7 @@ void Perceptron<LearnPolicy, WeightInitializationPolicy, MatType>::Classify(
for (size_t i = 0; i < test.n_cols; ++i)
{
tempLabelMat = weights.t() * test.col(i) + biases;
tempLabelMat.max(maxIndex);
maxIndex = tempLabelMat.index_max();
predictedLabels(i) = maxIndex;
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/mlpack/methods/radical/radical_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ inline typename MatType::elem_type Radical::Apply2D(const MatType& matX,
values(i) = Vasicek(candidateY1, m) + Vasicek(candidateY2, m);
}

arma::uword indOpt = 0;
values.min(indOpt); // we ignore the return value; we don't care about it
arma::uword indOpt = values.index_min();
return (indOpt / (ElemType) angles) * M_PI / 2.0;
}

Expand Down
3 changes: 1 addition & 2 deletions src/mlpack/methods/random_forest/random_forest_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,7 @@ void RandomForest<

// Find maximum element after renormalizing probabilities.
probabilities /= trees.size();
arma::uword maxIndex = 0;
probabilities.max(maxIndex);
arma::uword maxIndex = probabilities.index_max();

// Set prediction.
prediction = (size_t) maxIndex;
Expand Down
Loading