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

Wrap her, syr, her2, syr2 blas routines #3984

Merged
merged 2 commits into from
Sep 23, 2014
Merged

Conversation

ewmoore
Copy link
Member

@ewmoore ewmoore commented Sep 10, 2014

As a start toward including the rest of the currently unwrapped blas routines, I've wrapped these.

I'd like some feedback here on what we actually want out of these wrappers. Specifically, what we have doesn't seem particularly consistent from function to function (some have offx, some don't, etc.)

There also seem to be some issues (cf. #3983) and it looks to me like x and y in ger shouldn't really have the overwrite flag, since those are input only. I haven't changed that, since it would change the signature which we probably can't from a back compat standpoint.

@ewmoore
Copy link
Member Author

ewmoore commented Sep 10, 2014

Admittedly these routines don't seem particularly vital, and we'd probably like the various packed matrix-vector products more, but since I hadn't used f2py before I though I would start with some that seemed easier than gbmv.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 57885a3 on ewmoore:wrap_blas into 8b8531d on scipy:master.

@insertinterestingnamehere
Copy link
Member

It would be good to add these subroutines to the list at the top of blas.py so they are shown in the docs.

@ewmoore
Copy link
Member Author

ewmoore commented Sep 12, 2014

Pushed a commit that adds them to the list in blas.py

integer, intent(in), optional :: offx = 0
integer, intent(in), optional :: incx = 1

integer, intent(in), optional :: n = (len(x)-1-offx)/abs(incx)+1
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a depend(x, offs) --- I vaguely remember some problems from missing these. Edit: yeah, you have it in syr2 below

Copy link
Member

Choose a reason for hiding this comment

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

Yes, all variables used need the correspoding depends() statement, or
the computations may be done in an incorrect order.

Copy link
Member

Choose a reason for hiding this comment

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

Furthermore, if a depends on b and b depends on c, c needs depend(a, b) --- it seems f2py does not follow the dependency chain. cf ev-br@a38faa5

@ewmoore
Copy link
Member Author

ewmoore commented Sep 22, 2014

Updated.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6ad3686 on ewmoore:wrap_blas into * on scipy:master*.

@ev-br
Copy link
Member

ev-br commented Sep 23, 2014

LGTM, I'll merge this.

Meanwhile, which routines do you think need offx?
One other thing I think would be nice to have is to allow the optional lda argument to make it possible to work with submatrices.

ev-br added a commit that referenced this pull request Sep 23, 2014
Wrap her, syr, her2, syr2 blas routines
@ev-br ev-br merged commit 7c0c780 into scipy:master Sep 23, 2014
@ev-br ev-br added this to the 0.15.0 milestone Sep 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants