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

Dimension of argument match for matching-based scaling methods is unclear for non-square matrices #207

Closed
mjacobse opened this issue Jun 16, 2024 · 4 comments · Fixed by #209
Labels

Comments

@mjacobse
Copy link
Collaborator

mjacobse commented Jun 16, 2024

Documentation says match(n) (auction_scale_unsym, hungarian_scale_unsym), code says match(m) (auction_scale_unsym_int32, auction_scale_unsym_int64, hungarian_scale_unsym_int32, hungarian_scale_unsym_int64).

The tests pass an oversized match(maxn) with m, n <= maxn and the examples are using a square matrix, so they do not really provide clarification either.

Indeed, neither match(m) nor match(n) seem to work correctly in all cases. For both there are settings with either m < n or m > n that lead to memory access violations or uninitialized returned values.

@jfowkes
Copy link
Contributor

jfowkes commented Jun 17, 2024

Thanks @mjacobse, what a mess. It's not clear to me what the correct setting should be here.

@jfowkes
Copy link
Contributor

jfowkes commented Jun 17, 2024

@nimgould any ideas on what was intended here?

@jfowkes
Copy link
Contributor

jfowkes commented Jun 17, 2024

@nimgould says that the documentation is incorrect and the code is correct so it should be match(m) in the docs.

mjacobse added a commit to mjacobse/spral that referenced this issue Jun 18, 2024
Probably copy-paste error from symmetric cases. Fixes ralna#207
@mjacobse
Copy link
Collaborator Author

Yeah that makes sense. On further inspection, the only memory issues I can find when using m are with singular matrices and special cases of #200, so we can discuss and handle those there.

jfowkes pushed a commit that referenced this issue Jun 18, 2024
Probably copy-paste error from symmetric cases. Fixes #207
mjacobse added a commit to mjacobse/spral that referenced this issue Jun 18, 2024
cnt counts column-wise, so should be checked up to the number of columns
a%n, not the number of rows a%m. Probably copy-paste error from
symmetric case. Noticed when doing tests for ralna#207 by allocating the
exact sizes for each randomly generated matrix rather than the oversized
maximum sizes.
mjacobse added a commit to mjacobse/spral that referenced this issue Jun 18, 2024
cnt counts column-wise, so should be checked up to the number of columns
a%n, not the number of rows a%m. Noticed when doing tests for ralna#207 by
allocating the exact sizes for each randomly generated matrix rather than
the oversized maximum sizes.
jfowkes pushed a commit that referenced this issue Jun 19, 2024
cnt counts column-wise, so should be checked up to the number of columns
a%n, not the number of rows a%m. Noticed when doing tests for #207 by
allocating the exact sizes for each randomly generated matrix rather than
the oversized maximum sizes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants