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

Added element-wise exponentiation to dense NMatrix #106

Merged
merged 2 commits into from
Jul 19, 2013
Merged

Added element-wise exponentiation to dense NMatrix #106

merged 2 commits into from
Jul 19, 2013

Conversation

agarie
Copy link
Member

@agarie agarie commented Jul 19, 2013

This commit adds element-wise exponentiation to dense NMatrices through
the ** operator. For example:

n = NMatrix.new(:dense, 2, [1, 2, 3, 4], :float64)
n = n ** 2
# n will be [1, 4, 9, 16]

I think I changed all the relevant places, but I'm pretty sure there
are some cases in which my implementation will blow up. For one, I'm not
taking care of the resulting NMatrix's dtype, leaving it the same as the
left side NMatrix.

The problem with this is that, even if my NMatrix is :int64, I can have
a floating-point exponent, e.g. NMatrix.new(2, 3) ** 3.14. The
solution probably involves doing some checks before delegating the
calculation to ew_op_switch() (see dense.cpp#772).

Can anybody see more problems with this implementation? I want to fix
the issues with element-wise operations in list matrices (#42) and
implement the operations still needed, i.e. modulo and exponentiation.
For this, I need more experience dealing with these parts of NMatrix. :)

I'm sure there's a way to cleanup the codebase a bit regarding this as
well.

There are some changes that are related to spacing (removing tabs for
true spaces). They shouldn't hamper this PR's legibility and are a Good
Thing for the project, so let's leave them here. Also, some places of
the codebase need a bit more of care & love regarding comment
positioning and spacing, but that's a discussion for another day...

(yes, it's compiling and passing the new spec with rake compile && rake spec)

agarie added 2 commits July 18, 2013 23:35
This commit adds element-wise exponentiation to dense NMatrices through
the `**` operator. For example:

    n = NMatrix.new(:dense, 2, [1, 2, 3, 4], :float64)
    n = n ** 2
    # n will be [1, 4, 9, 16]

I *think* I changed all the relevant places, but I'm pretty sure there
are some cases in which my implementation will blow up. For one, I'm not
taking care of the resulting NMatrix's dtype, leaving it the same as the
left side NMatrix.

The problem with this is that, even if my NMatrix is :int64, I can have
a floating-point exponent, e.g. `NMatrix.new(2, 3) ** 3.14`. The
solution probably involves doing some checks before delegating the
calculation to ew_op_switch() (see dense.cpp#772).

Can anybody see more problems with this implementation? I want to fix
the issues with element-wise operations in list matrices (#42) and
implement the operations still needed, i.e. modulo and exponentiation.
For this, I need more experience dealing with these parts of NMatrix. :)

I'm sure there's a way to cleanup the codebase a bit regarding this as
well.

There are some changes that are related to spacing (removing tabs for
true spaces). They shouldn't hamper this PR's legibility and are a Good
Thing for the project, so let's leave them here. Also, some places of
the codebase need a bit more of care & love regarding comment
positioning and spacing, but that's a discussion for another day...
I forgot to add this file in my last commit. It adds the capability to
generate a ttable with the element-wise power operator.
@ghost ghost assigned translunar Jul 19, 2013
@translunar
Copy link
Member

@agarie This is absolutely awesome. Great, great work.

So, in considering the ** 3.14 issue, here are my thoughts:

First of all, you can have a Fixnum * Rationalwhich produces an integer (but NOT a Fixnum) -- for example,16*(5.quo(2))=1024.0`, which is a Float.

I think the proper thing to do for ** Fixnum is return a Fixnum matrix of some kind. I can't decide how strict we should be with types here, though. For example, 10 ** 25 returns a Bignum. What happens if someone calls **25 on an :int8 matrix? What about an :int64 matrix? It seems like it would make the most amount of sense to do upcasting ONLY when the matrix is of type :object -- then Ruby handles the upcasting automatically. We should force the user to do a cast if they want a power operation on an :int32 to yield an :int64. Otherwise it's going to severely slow down our matrix operations -- we'd have to run the operation once with a cast and once without and see if it fails an equality test, basically.

So what happens if we do a Fixnum to the power of a Rational or Float? This is where I'm not quite certain. Our options:

  1. Implicitly cast any intx matrix to a :float64, to sort of mimic Ruby behavior.
  2. Refuse to do the operation. Inform the user, via raise, that he or she needs to call cast first to provide further instruction.
  3. Do the operation with the float value, but then take the floor of the float result so that the actual returned matrix is still of the original dtype.

I dislike option 3. I think it's inconsistent with Ruby. I also dislike option 1, because I think the :object dtype is the one that is supposed to behave like Ruby. I prefer option 2 because it allows the user maximal control -- the user can get behavior 1 or 3 if he or she likes depending on where the cast call goes in the chain.

So, I'd say refuse to do such an operation.

translunar added a commit that referenced this pull request Jul 19, 2013
Added element-wise exponentiation to dense NMatrix
@translunar translunar merged commit 5ec48bd into SciRuby:master Jul 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants