Refactor binary-operations #11
Assignees
Labels
enhancement
New feature or request
Comments
|
I have just pushed a branch that implements greatly simplified versions of the functions in Some notes:
@beberman, are you able to work on completing the tests? I have converted a few to show how it's done; it's search and replace from here on. I've got some critical coursework to prepare, and if you could get these tests completed it will be a huge step forward for us, plus get another pair of eyes on the work. |
|
Sounds good. I'll take a look next week
…On Sat, Sep 2, 2023, 9:34 AM Steve Nunez ***@***.***> wrote:
I have just pushed a branch that implements greatly simplified versions of
the functions in binary-operations. This also introduces a new package
linalg. I propose that as we refactor, we move to a single name space and
drop linear-algebra and linear-algebra (or possibly have linalg as a
nickname if we like linear-algebra for stylistic reasons.
Some notes:
- It isn't feasible to have an external definition of the generic
functions due to packaging. Well, it's possible, but fragile and ugly. I
think it's simpler to just have both linalg and lla agree to use the
same function names, and then import on a selective basis, as was suggested
in #6 <#6>.
- I have dropped scaling parameters in the new implementations. A lot
of effort seems to have gone into making scaling various parameters as part
of the internals of the function call. I think it's better to make it
explicit: call scale on an array before passing it. See the binops
tests for an example of what the new style looks like.
- I have left the original functions in the same package. As well I've
only added tests for the new functions that mirror the old ones. The reason
is that we may need them during the transition, especially with the
specialised matrix types (hermitian, etc). Although, I question whether
they are worth keeping. I'll open an issue for this.
@beberman <https://github.com/beberman>, are you able to work on
completing the tests? I have converted a few to show how it's done; it's
search and replace from here on. I've got some critical coursework to
prepare, and if you could get these tests completed it will be a huge step
forward for us, plus get another pair of eyes on the work.
—
Reply to this email directly, view it on GitHub
<#11 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACM4YURKQF27PCH6YYF27W3XYMYWRANCNFSM6AAAAAA3YF6LZ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Steve - before I update the tests I would like to make sure I understand a
couple of things
1) It appears that all the destructive operations nadd, nmultiply ect. will
no longer be supported. All operations now generate a new array. Clearly
there are memory costs to this decision. Does it matter? I'm not certain
which has faster cpu performance.
2) The code in src/kernel/binary-operations has the
scaled-binary-operations commented out. This actually leads to this entire
file being non-functional so it should be deleted?
3) The file src/linalg was created (copied) from lla and put in the linalg
name space. Why is it in that name space? I see that you say this
introduced the new package but why in this refactor did you introduce that
new package? I think the decision to either stick with linear-algebra or
move to linalg should be made now. Then this package can have consistent
names and then those names can be used in the tests.
I would like to get the src code into the final form before rewriting the
tests. A lot of the tests can just be deleted. There is no reason to test
scaling the first, second, and both vectors before adding since scale is
now destructive it just needs to be tested individually and then add and
multiply can be tested individually.
…-Brian
On Sat, Sep 2, 2023 at 9:34 AM Steve Nunez ***@***.***> wrote:
I have just pushed a branch that implements greatly simplified versions of
the functions in binary-operations. This also introduces a new package
linalg. I propose that as we refactor, we move to a single name space and
drop linear-algebra and linear-algebra (or possibly have linalg as a
nickname if we like linear-algebra for stylistic reasons.
Some notes:
- It isn't feasible to have an external definition of the generic
functions due to packaging. Well, it's possible, but fragile and ugly. I
think it's simpler to just have both linalg and lla agree to use the
same function names, and then import on a selective basis, as was suggested
in #6 <#6>.
- I have dropped scaling parameters in the new implementations. A lot
of effort seems to have gone into making scaling various parameters as part
of the internals of the function call. I think it's better to make it
explicit: call scale on an array before passing it. See the binops
tests for an example of what the new style looks like.
- I have left the original functions in the same package. As well I've
only added tests for the new functions that mirror the old ones. The reason
is that we may need them during the transition, especially with the
specialised matrix types (hermitian, etc). Although, I question whether
they are worth keeping. I'll open an issue for this.
@beberman <https://github.com/beberman>, are you able to work on
completing the tests? I have converted a few to show how it's done; it's
search and replace from here on. I've got some critical coursework to
prepare, and if you could get these tests completed it will be a huge step
forward for us, plus get another pair of eyes on the work.
—
Reply to this email directly, view it on GitHub
<#11 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACM4YURKQF27PCH6YYF27W3XYMYWRANCNFSM6AAAAAA3YF6LZ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
The way it's written now, all the operations are destructive, they're just not named as such. I don't think the destructive/non-consing distinction makes sense when dealing with arrays. I am not aware of any other libraries (Numpy, R) that work this way either. It seems a relic of old-lisp where everything was cons based and those were expensive operations. Happy to reconsider though.
It can be deleted. I left it there as a reminder in case we had to go back and refer to them whilst updating the specialised matrix types, but I supposed we can also use git history for that.
The intention is to merge |
|
Numpy by default is non-destructive (
https://numpy.org/doc/stable/reference/generated/numpy.add.html). An array
target can be provided that could be one of the arrays. So I think the
default should be non-destructive.
Linalg is fine for me. Let's just move everything to that namespace.
…On Wed, Sep 13, 2023 at 8:43 AM Steve Nunez ***@***.***> wrote:
1. It appears that all the destructive operations nadd, nmultiply ect.
will no longer be supported. All operations now generate a new array.
Clearly there are memory costs to this decision. Does it matter? I'm not
certain which has faster cpu performance.
The way it's written now, all the operations *are* destructive, they're
just not named as such. I don't think the destructive/non-consing
distinction makes sense when dealing with arrays. I am not aware of any
other libraries (Numpy, R) that work this way either. It seems a relic of
old-lisp where everything was cons based and those were expensive
operations. Happy to reconsider though.
1. The code in src/kernel/binary-operations has the
scaled-binary-operations commented out. This actually leads to this entire
file being non-functional so it should be deleted?
It can be deleted. I left it there as a reminder in case we had to go back
and refer to them whilst updating the specialised matrix types, but I
supposed we can also use git history for that.
1. The file src/linalg was created (copied) from lla and put in the
linalg name space. Why is it in that name space? I see that you say this
introduced the new package but why in this refactor did you introduce that
new package? I think the decision to either stick with linear-algebra or
move to linalg should be made now. Then this package can have consistent
names and then those names can be used in the tests. I would like to get
the src code into the final form before rewriting the tests. A lot of the
tests can just be deleted. There is no reason to test scaling the first,
second, and both vectors before adding since scale is now destructive it
just needs to be tested individually and then add and multiply can be
tested individually.
The intention is to merge linear-algebra and linear-algebra-kernel into a
single namespace. Since we were talking about linalg, I chose that, but
it could just as easily be linear-algebra. If you want to complete the
namespace refactoring, I agree that now's a good time to do so. As for
scaling: agreed. Some of the tests he wrote don't make any sense and are
duplicates to a large degree and we can greatly simplify the tests too.
—
Reply to this email directly, view it on GitHub
<#11 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACM4YUWJ2UXDQMEJPRCLZOLX2GTAXANCNFSM6AAAAAA3YF6LZ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'm also confused about what's happening with the underlying types here.
It appears that your new code relies on the built in vector type which
eliminates the distinction between row and column vectors (I'm ok with
that). It is also using the built-in array type. However the code in
data-vector at least and potentially the other type files is still using
the linear-algebra types. So how would you like to refactor that code?
Let's resolve this before testing.
Best,
-Brian
On Wed, Sep 13, 2023 at 9:08 AM Brian Eberman ***@***.***>
wrote:
… Numpy by default is non-destructive (
https://numpy.org/doc/stable/reference/generated/numpy.add.html). An
array target can be provided that could be one of the arrays. So I think
the default should be non-destructive.
Linalg is fine for me. Let's just move everything to that namespace.
On Wed, Sep 13, 2023 at 8:43 AM Steve Nunez ***@***.***>
wrote:
>
> 1. It appears that all the destructive operations nadd, nmultiply
> ect. will no longer be supported. All operations now generate a new array.
> Clearly there are memory costs to this decision. Does it matter? I'm not
> certain which has faster cpu performance.
>
> The way it's written now, all the operations *are* destructive, they're
> just not named as such. I don't think the destructive/non-consing
> distinction makes sense when dealing with arrays. I am not aware of any
> other libraries (Numpy, R) that work this way either. It seems a relic of
> old-lisp where everything was cons based and those were expensive
> operations. Happy to reconsider though.
>
>
> 1. The code in src/kernel/binary-operations has the
> scaled-binary-operations commented out. This actually leads to this entire
> file being non-functional so it should be deleted?
>
> It can be deleted. I left it there as a reminder in case we had to go
> back and refer to them whilst updating the specialised matrix types, but I
> supposed we can also use git history for that.
>
>
> 1. The file src/linalg was created (copied) from lla and put in the
> linalg name space. Why is it in that name space? I see that you say this
> introduced the new package but why in this refactor did you introduce that
> new package? I think the decision to either stick with linear-algebra or
> move to linalg should be made now. Then this package can have consistent
> names and then those names can be used in the tests. I would like to get
> the src code into the final form before rewriting the tests. A lot of the
> tests can just be deleted. There is no reason to test scaling the first,
> second, and both vectors before adding since scale is now destructive it
> just needs to be tested individually and then add and multiply can be
> tested individually.
>
> The intention is to merge linear-algebra and linear-algebra-kernel into
> a single namespace. Since we were talking about linalg, I chose that,
> but it could just as easily be linear-algebra. If you want to complete
> the namespace refactoring, I agree that now's a good time to do so. As for
> scaling: agreed. Some of the tests he wrote don't make any sense and are
> duplicates to a large degree and we can greatly simplify the tests too.
>
> —
> Reply to this email directly, view it on GitHub
> <#11 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACM4YUWJ2UXDQMEJPRCLZOLX2GTAXANCNFSM6AAAAAA3YF6LZ4>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
Correct, there's no benefit to a specialised dense array type, only added complexity. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This consists of:
Then implement the same in LLA so the API's match.
The text was updated successfully, but these errors were encountered: