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.

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

Fix binding handling of transposed matrix parameters #3327

Merged
merged 8 commits into from Dec 6, 2022

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Dec 1, 2022

Digging into #3324, I realized that none of our bindings (except command-line programs) have handling for parameters declared as PARAM_TMATRIX_IN() (i.e. a transposed matrix). LARS happens to be the only binding that actually uses this parameter type, and so what happens is that in languages other than command-line bindings (such as Python), the user's data will be unexpectedly transposed.

This PR adds support for transposed matrices to Python, Go, R, and Julia bindings, which will in turn fix the strange behavior specific to LARS that was encountered in #3324 (and probably by other users).

For each language, this boils down to:

  • adding a test for a PARAM_TMATRIX_IN parameter
  • handling the noTranspose parameter from ParamData while generating the bindings
  • updating the glue code between languages so it can support transposing if needed

There are also a couple other tiny fixes in here:

  • fix some deprecation warnings from numpy in matrix_utils.py
  • fix CMake configuration so that python_bindings_test is not added unless BUILD_TESTS is ON
  • fix Go configuration so that go_bindings_test is not added unless BUILD_TESTS is ON

Copy link

@mlpack-bot mlpack-bot 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 rcurtin merged commit 4561adc into mlpack:master Dec 6, 2022
@rcurtin rcurtin deleted the tmatrix-binding-fixes branch December 6, 2022 11:54
@rcurtin rcurtin mentioned this pull request Dec 23, 2022
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.

None yet

2 participants