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

Check input matrix orientation when loading problem data #141

Open
wants to merge 2 commits into
base: stable/1.2
Choose a base branch
from

Conversation

yuxies
Copy link

@yuxies yuxies commented Sep 3, 2024

To fix the bug in #134: since loadProblemData() expects a column-oriented data matrix, we consider adding a check to the beginning of the function and creating a reversed copy if a row-oriented matrix is detected.

@tkralphs
Copy link
Member

tkralphs commented Sep 3, 2024

Copying and changing the orientation are expensive operations, do we really need to make the copy? Don't we won the matrix? Also, I wonder if we should just throw an error instead, forcing the user to give us the right thing? Otherwise, they may not even realize that they are constructing the matrix in the wrong orientation to begin with.

@yuxies
Copy link
Author

yuxies commented Sep 3, 2024

We have to make a copy to change the col/row order; the function loadProblemData() does not own the matrix since it is passed in with the const keyword.

I wonder if we should just throw an error instead, forcing the user to give us the right thing? Otherwise, they may not even realize that they are constructing the matrix in the wrong orientation to begin with.

I was also thinking about this strategy to avoid changing any part of the function itself. It is natural for the user to create a row matrix when they input data by adding rows to the matrix. So if we throw an error, we can inform the user that they are creating a row matrix, while the function defaults to using a column matrix.

@tkralphs
Copy link
Member

tkralphs commented Sep 3, 2024

I guess that throwing an error is better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants