friendly / matlib

Matrix Functions for Teaching and Learning Linear Algebra and Multivariate Statistics
http://friendly.github.io/matlib/
65 stars 16 forks source link

matlib::echelon fails with matrix with single row #30

Closed JanaJarecki closed 4 years ago

JanaJarecki commented 4 years ago

Hej, the matlib::echelon function cannot handle matrices with one row. While this may be usually not needed at all, I am using the echelon function in a package where it is possible that sometimes the A matrix can indeed have one row only. I would prefer the function not to break the code run with an error.

A <- matrix(c(0,1,1,0), 2)
b <- 1:2
matlib::echelon(A,b) # works
matlib::echelon(A[1, , drop = F],b[1]) #fails
# should return sth like
#      [,1] [,2]  [,3]                                                                                    
  [1,]    0    1    1
john-d-fox commented 4 years ago

Dear Jana,

I agree that echelon() should work for edge cases like a one-row (or one-column) matrix. The problem actually is in gaussianElimination(). I'll look into it when I have a chance.

Thanks for the bug report.

John

-----Original Message----- From: Jana Jarecki notifications@github.com Sent: Wednesday, March 25, 2020 1:25 PM To: friendly/matlib matlib@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [friendly/matlib] matlib::echelon fails with matrix with single row (#30)

Hej, the matlib::echelon function cannot handle matrices with one row. While this may be usually not needed at all, I am using the echelon function in a package where I would prefer the function not to break.

A <- matrix(c(0,1,1,0), 2) b <- 1:2 matlib::echelon(A,b) # works matlib::echelon(A[1, , drop = F],b[1]) #fails # should return sth like

[,1] [,2] [,3]

[1,] 0 1 1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/30 , or unsubscribe https://github.com/notifications/unsubscribe- auth/ADLSAQST6WTTPOF5XCPQKCLRJI46RANCNFSM4LTT2OXQ . https://github.com/notifications/beacon/ADLSAQVXO3K5REYTDDEUGHDRJI46RA5CN FSM4LTT2OX2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IYKCNTQ.gif

friendly commented 4 years ago

True: the error is triggered in gaussianElimination

> gaussianElimination(A[1, , drop=F], b[1])
Error in apply(A[, 1:m], 1, function(x) max(abs(x)) <= tol) : 
  dim(X) must have a positive length
>
friendly commented 4 years ago

And, there is an arguable case for this in terms of showEqn with a simple solution.

> showEqn(A[1,,drop=F], b[1])
0*x1 + 1*x2  =  1 
john-d-fox commented 4 years ago

Hi Michael,

I hope that all is well with you, Martha, and the rest of your family. Bonnie and I are back in Toronto, having returned from Florida on Sunday, and of course are in self-isolation. We're fine.

Unless you feel it's urgent and want to fix it yourself, I'll get to this after I finish some other work. I think it should be simple to fix even if just by special-casing one-row and one-column matrices. It would be nice, of course, to have a more elegant solution.

Best, John

On Mar 25, 2020, at 3:51 PM, Michael Friendly notifications@github.com wrote:

And, there is an arguable case for this in terms of showEqn with a simple solution.

showEqn(A[1,,drop=F], b[1]) 0x1 + 1x2 = 1

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

john-d-fox commented 4 years ago

Hi,

I think this is now fixed in matlib on GitHub (requiring only two small changes). I also bumped the version number to 0.9.3 and updated the NEWS.md file.

Best, John

On Mar 25, 2020, at 1:24 PM, Jana Jarecki notifications@github.com wrote:

Hej, the matlib::echelon function cannot handle matrices with one row. While this may be usually not needed at all, I am using the echelon function in a package where I would prefer the function not to break.

A <- matrix(c(0,1,1,0), 2) b <- 1:2 matlib::echelon(A,b) # works matlib::echelon(A[1, , drop = F],b[1]) #fails

should return sth like

[,1] [,2] [,3]

[1,] 0 1 1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

JanaJarecki commented 4 years ago

Works like a charm for me

john-d-fox commented 4 years ago

Dear Jana,

Thanks for check it, and again for the bug report. Michael can decide when we've accumulated enough changes in matlib to justify a new CRAN version.

Best, John

-----Original Message----- From: Jana Jarecki notifications@github.com Sent: Friday, March 27, 2020 1:11 PM To: friendly/matlib matlib@noreply.github.com Cc: Fox, John jfox@mcmaster.ca; Comment comment@noreply.github.com Subject: Re: [friendly/matlib] matlib::echelon fails with matrix with single row (#30)

Works like a charm for me

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/30#issuecomment-605118816 , or unsubscribe https://github.com/notifications/unsubscribe- auth/ADLSAQTCZZ53GGKDOAMOFDDRJTMZTANCNFSM4LTT2OXQ . https://github.com/notifications/beacon/ADLSAQXLNTEQTYSPMOSFFGDRJTMZTA5CN FSM4LTT2OX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEQI WCYA.gif

friendly commented 4 years ago

Looks good to me, but I'l have to sort out the devel-0.9-3 branch mentioned in #28

john-d-fox commented 4 years ago

Hi Michael,

Sorry, I forgot about that. It might be simplest just to revert my changes after you look at what I did to gaussianElimination(), which are just small changes to two lines of code. You could then just apply these changes to the devel branch.

John

On Mar 27, 2020, at 11:37 PM, Michael Friendly notifications@github.com wrote:

Looks good to me, but I'l have to sort out the devel-0.9-3 branch mentioned in #28

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

friendly commented 4 years ago

I did some more testing. gaussianElimination and friends work well with 1-row systems. This part seems to be done.

However plotEqn gives an error for equations involving only x1, It does give the correct plot, and the problem has to do with labeling the equation.

# works for 1-row systems directly

A1 <- matrix(c(1, 0), nrow=1)
b1 = 2
showEqn(A1, b1, simplify=TRUE)
gaussianElimination(A1, b1)

# plotEqn give an errors for this case. It gives the right
# plot, but generates an error
plotEqn(A1, b1)
#> plotEqn(A1, b1)
#x1  =  2 
#Error in plotEqn(A1, b1) : object 'y' not found
#>

image

# works for 1-row systems (issue # 30)
A2 <- matrix(c(1, 1), nrow=1)
b2 = 2
gaussianElimination(A2, b2)
showEqn(A2, b2)
# plotEqn works for this case
plotEqn(A2, b2)

image

john-d-fox commented 4 years ago

Hi Michael,

I made a small change today to gaussianElimination() to make the fix for the one-column case a little more transparent.

Do you want to fix plotEqn() or shall I? I noticed, BTW, that plotEqn() doesn't print proper subscripts for the xs. We could use, e.g., expression(x[1]) to make the labels look nicer.

Best, John

On Mar 29, 2020, at 4:46 PM, Michael Friendly notifications@github.com wrote:

I did some more testing. gaussianElimination and friends work well with 1-row systems. This part seems to be done.

However plotEqn gives an error for equations involving only x1, It does give the correct plot, and the problem has to do with labeling the equation.

works for 1-row systems directly

A1 <- matrix(c(1, 0), nrow=1) b1 = 2 showEqn(A1, b1, simplify=TRUE) gaussianElimination(A1, b1)

plotEqn give an errors for this case. It gives the right

plot, but generates an error

plotEqn(A1, b1)

> plotEqn(A1, b1)

x1 = 2

Error in plotEqn(A1, b1) : object 'y' not found

>

works for 1-row systems (issue # 30)

A2 <- matrix(c(1, 1), nrow=1) b2 = 2 gaussianElimination(A2, b2) showEqn(A2, b2)

plotEqn works for this case

plotEqn(A2, b2)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

friendly commented 4 years ago

I tried debug() on plotEqn. I see what the problem is, and fixed it, sort of.

The error occurs for equations of the form x1 = c, i.e., when A[i,2] = 0. The label for such a line could be positioned at (x=c, y=anywhere).

I modified the code, and it doesn't produce the error. Can you take a look at the latest commit?

My test case is:

A1 <- matrix(c(1, 0), nrow=1)
b1 = 2
showEqn(A1, b1, simplify=TRUE)
plotEqn(A1, b1)

This gives

image

john-d-fox commented 4 years ago

Hi,

-----Original Message----- From: Michael Friendly notifications@github.com Sent: Monday, March 30, 2020 11:08 AM To: friendly/matlib matlib@noreply.github.com Cc: Fox, John jfox@mcmaster.ca; Comment comment@noreply.github.com Subject: Re: [friendly/matlib] matlib::echelon fails with matrix with single row (#30)

I tried debug() on plotEqn. I see what the problem is, and fixed it, sort of.

The error occurs for equations of the form x1 = c, i.e., when A[i,2] = 0. The label for such a line could be positioned at (x=c, y=anywhere).

I modified the code, and it doesn't produce the error. Can you take a look at the latest commit?

Sure. I should be able to get to this later today.

Should I also try to improve the typesetting of the labels?

John

My test case is:

A1 <- matrix(c(1, 0), nrow=1) b1 = 2 showEqn(A1, b1, simplify=TRUE) plotEqn(A1, b1)

This gives

https://user-images.githubusercontent.com/456353/77928557-903f5b00-7276- 11ea-8893-6fd4a7e9552e.png

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/30#issuecomment-606057117 , or unsubscribe https://github.com/notifications/unsubscribe- auth/ADLSAQVDFU4EUICAMUGCBNTRKCYUPANCNFSM4LTT2OXQ . https://github.com/notifications/beacon/ADLSAQVYR7SDVOD46E2RKO3RKCYUPA5CN FSM4LTT2OX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEQP 3FHI.gif

john-d-fox commented 4 years ago

Hi,

This looks right to me. I also pushed a change to the default labelling so that subscripts are properly typeset. You can revert it if you don't like it.

That just leaves the hex sticker question ;)

John

On Mar 30, 2020, at 11:07 AM, Michael Friendly notifications@github.com wrote:

I tried debug() on plotEqn. I see what the problem is, and fixed it, sort of.

The error occurs for equations of the form x1 = c, i.e., when A[i,2] = 0. The label for such a line could be positioned at (x=c, y=anywhere).

I modified the code, and it doesn't produce the error. Can you take a look at the latest commit?

My test case is:

A1 <- matrix(c(1, 0), nrow=1) b1 = 2 showEqn(A1, b1, simplify=TRUE) plotEqn(A1, b1)

This gives

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

friendly commented 4 years ago

Great! Let's call this done.

If there are no further changes, I'll begin to roll out the current version to CRAN.