Skip to content
This repository was archived by the owner on Dec 10, 2018. It is now read-only.

Added Triangular type, At and Set methods, and tests#77

Closed
btracey wants to merge 5 commits intomasterfrom
addtri
Closed

Added Triangular type, At and Set methods, and tests#77
btracey wants to merge 5 commits intomasterfrom
addtri

Conversation

@btracey
Copy link
Copy Markdown
Member

@btracey btracey commented Jan 13, 2015

No description provided.

Comment thread mat64/triangular.go
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naive comment: isn't "cleaner" to just spell it out
_ Matrix = (*Triangular)(nil)

admittedly more verbose, but no "pollution" of the global scope.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm following the pattern that exists there now. We can change it if there's a better way, but consistency is better.

@btracey
Copy link
Copy Markdown
Member Author

btracey commented Jan 13, 2015

Fixed comments and added a comment to NewTriangular about the underlying data. PTAL

@jonlawlor
Copy link
Copy Markdown
Contributor

Is QR going to be changed to:

type QRFactor struct {
    QR    *Triangular
    rDiag []float64
}

@btracey
Copy link
Copy Markdown
Member Author

btracey commented Jan 13, 2015

I would vote it gets changed in some form. You can see my thoughts on Eigen at Issue #79 , and I have similar thoughts for Cholesky and QRFactor. I'm not a dictator though, we'll need to discuss how we want these to look. Getting the basic types in will make it easier to think about.

@btracey
Copy link
Copy Markdown
Member Author

btracey commented Jan 13, 2015

We should also consider if we want the Upper and Lower constants, or if we would rather have a boolean where true == Upper. There are only two possibilities, it would eliminate a panic possibility, and a boolean would match what we've chosen for Trans. However, it seemed to be that Upper and Lower read better, and it's not clear a priori which one should be true.

@jonlawlor
Copy link
Copy Markdown
Contributor

You can type Upper and Lower to boolean and callers can just use true and false. http://play.golang.org/p/9FKN8h4SbT

@btracey
Copy link
Copy Markdown
Member Author

btracey commented Jan 15, 2015

Removed TriType and made the field a Boolean. We can revisit if we want to create a typed Boolean, but this is in line with the current Trans behavior.

@jonlawlor
Copy link
Copy Markdown
Contributor

That bool change looks good to me; but I haven't reviewed all of the other changes in depth.

@btracey
Copy link
Copy Markdown
Member Author

btracey commented Jan 16, 2015

Pulled from master

@btracey
Copy link
Copy Markdown
Member Author

btracey commented Jan 30, 2015

Just made a new PR ( #94) that incorporates some changes from the symmetric discussion.

@btracey btracey closed this Jan 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants