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

mat: consider reconciling option passing in factorise methods #756

Closed
kortschak opened this issue Apr 30, 2017 · 6 comments · Fixed by #872
Closed

mat: consider reconciling option passing in factorise methods #756

kortschak opened this issue Apr 30, 2017 · 6 comments · Fixed by #872
Labels
stability affects API stability

Comments

@kortschak
Copy link
Member

At the moment there are three factorisation methods that take options, the methods on Eigen, EigenSym, GSVD and SVD. These take different approaches to specifying options for factorisation.

I'm not too sure whether these can be reconciled, or whether doing so would be a net benefit, but it is worth considering.

@btracey @vladimir-ch Views?

@vladimir-ch
Copy link
Member

Using ORed bitfields for specifying various options feels quite low level and out-of-place in a matrix package but it 1) works, 2) does not look terrible, and most importantly 3) unlike just true/false the individual bits have names that appear at the call site. So I might prefer the SVD approach over Eigen.

@vladimir-ch vladimir-ch transferred this issue from gonum/matrix Dec 9, 2018
@vladimir-ch vladimir-ch changed the title matrix/mat64: consider reconciling option passing in factorise methods mat: consider reconciling option passing in factorise methods Dec 9, 2018
@btracey
Copy link
Member

btracey commented Feb 2, 2019

I agree with @vladimir-ch that ORed bit-fields are out of place. It's not clear to me that reconciling the Factorize methods is a goal, but if it is I think the way to do it is to use the approach we took in samplemv. There, we moved the options into the struct type, and rectified the signatures. So, it would be

type SVD struct {
    UKind SVDKind
    VKind SVDKind 
   // contains filtered or unexported fields
}

func (svd *SVD) Factorize(a Matrix) (ok bool){...}

and be used as

svd := &mat.SVD{UKind: svd.Thin, VKind: svd.None}.Factorize(a)

In samplemv this is clearly a good idea because we wanted the types to all satisfy the same interface. It's not clear that is a useful feature here. I think if we do do this then we should make them satisfy the same interfaces though, in case that turns out to be useful. In particular, Cholesky should be FactorizeSym rather than just Factorize. One upside to this is that we could make the zero-values take the default (UKind and VKind default to Thin, vectors in EigenSym is true, etc.). This way a user who doesn't want to think about anything can use

svd := &mat.SVD{}.Factorize(a)

and it will work as expected.

@kortschak
Copy link
Member Author

There was more to that sentence.

@btracey
Copy link
Member

btracey commented Feb 2, 2019

I don't follow

@btracey
Copy link
Member

btracey commented Feb 2, 2019

Assuming you meant @vladimir-ch 's sentence, there was also more to my post?

@kortschak
Copy link
Member Author

I was responding to the first sentence, yes.

I don't think that there is a good justification for a common interface, but I do think there is a good reason for a general consistency in the approach taken to passing options. The first would be there to ensure code reusability between the factorising types, which I think is a non-goal since one factorisation is not the same as another factorisation (and the result accessors would be different anyway - so they should be part of the interface too if it existed which means there would not be a common interface). The existence of a consistent user interface (the signature vibe) means that learning one means that the user has the concepts necessary for rapidly learning others (the only difference being the exact semantics of the constants/parameters).

@btracey btracey added the stability affects API stability label Feb 15, 2019
btracey added a commit that referenced this issue Feb 20, 2019
btracey added a commit that referenced this issue Mar 23, 2019
* mat: change factorization inputs to use bit types

Fixes #756 and #748.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stability affects API stability
Projects
None yet
3 participants