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

PolyhedralGeometry: Enhance docs of solve_mixed #2798

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Conversation

lkastner
Copy link
Member

No description provided.

@HechtiDerLachs
Copy link
Collaborator

Seems reasonable, thanks for the work. The original issue which finally lead to this PR was the following: The docstring says we solve a system A*x = b under some condition. Here A and B are both matrices. Hence, given what I was used to from the context of lift-like commands from commutative algebra, I was expecting also x to be a matrix; probably the one who's j-th column multiplied with A becomes the j-th column of b. What was confusing me was that what I got was a matrix x which I could not even multiply with A! Example:

julia> A = ZZ[1 0; 1 -1]
[1    0]
[1   -1]

julia> B = ZZ[1; 1]
[1]
[1]

julia> C = ZZ[1 0; 0 1]
[1   0]
[0   1]

julia> x = solve_mixed(ZZMatrix, A, B, C)
[1   0]

julia> A*x
ERROR: Incompatible matrix dimensions
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] *(x::ZZMatrix, y::ZZMatrix)
   @ Nemo ~/.julia/packages/Nemo/Tj33G/src/flint/fmpz_mat.jl:312
 [3] top-level scope
   @ REPL[95]:1

This is something that I personally still find confusing. But I understand that a lot of demands have already gone into the design of this function and I am not the only one who is confused about something. The docstring now might have helped me better to overcome my confusion, so I think this is an improvement.

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #2798 (35b3545) into master (ebee98c) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2798      +/-   ##
==========================================
- Coverage   73.61%   73.56%   -0.05%     
==========================================
  Files         455      455              
  Lines       64469    64469              
==========================================
- Hits        47459    47429      -30     
- Misses      17010    17040      +30     
Files Changed Coverage
src/PolyhedralGeometry/solving_integrally.jl ø

@lkastner lkastner merged commit 92bf5cb into master Sep 14, 2023
@lkastner lkastner deleted the lk/docs/solve_mixed branch September 14, 2023 19:01
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