Skip Menu |

This queue is for tickets about the Math-Vector-Real CPAN distribution.

Report information
The Basics
Id: 88487
Status: rejected
Priority: 0/
Queue: Math-Vector-Real

People
Owner: Nobody in particular
Requestors: djerius [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 0.10
Fixed in: (no value)



Subject: objects constructed via ops subs aren't blessed into parent object's class
In subclasses of Math::Vector::Real, objects created while performing any operations are blessed into Math::Vector::Real, not into the subclass. For example: $ reply Show quoted text
0> package VR { use base 'Math::Vector::Real' }
Show quoted text
1> VR->new
$res[0] = bless( [], 'VR' ) Show quoted text
2> VR->new * 3
$res[1] = bless( [], 'Math::Vector::Real' ) I expected the latter operation to result in an object blessed into VR. Thanks, Diab
On Sat Sep 07 17:37:46 2013, DJERIUS wrote: Show quoted text
> In subclasses of Math::Vector::Real, objects created while performing > any operations are blessed into Math::Vector::Real, not into the > subclass.
Attached is a patch implementing this behavior.
Subject: pp.patch
# bless constructed objects in proper class # # To apply this patch: # STEP 1: Chdir to the source directory. # STEP 2: Run the 'applypatch' program with this patch file as input. # # If you do not have 'applypatch', it is part of the 'makepatch' package # that you can fetch from the Comprehensive Perl Archive Network: # http://www.perl.com/CPAN/authors/Johan_Vromans/makepatch-x.y.tar.gz # In the above URL, 'x' should be 2 or higher. # # To apply this patch without the use of 'applypatch': # STEP 1: Chdir to the source directory. # If you have a decent Bourne-type shell: # STEP 2: Run the shell with this file as input. # If you don't have such a shell, you may need to manually create # the files as shown below. # STEP 3: Run the 'patch' program with this file as input. # # These are the commands needed to create/delete files/directories: # touch 't/subclass.t' chmod 0664 't/subclass.t' # # This command terminates the shell and need not be executed manually. exit # #### End of Preamble #### #### Patch data follows #### diff -c 'Math-Vector-Real-0.10.orig/MANIFEST' 'Math-Vector-Real-0.10/MANIFEST' Index: ./MANIFEST *** ./MANIFEST Tue Jul 17 07:50:36 2012 --- ./MANIFEST Sat Sep 7 19:01:54 2013 *************** *** 3,8 **** --- 3,9 ---- MANIFEST README t/Math-Vector-Real.t + t/subclass.t t/pods.t lib/Math/Vector/Real.pm examples/dist_to_line.pl diff -c 'Math-Vector-Real-0.10.orig/lib/Math/Vector/Real.pm' 'Math-Vector-Real-0.10/lib/Math/Vector/Real.pm' Index: ./lib/Math/Vector/Real.pm *** ./lib/Math/Vector/Real.pm Wed Jul 4 09:17:09 2012 --- ./lib/Math/Vector/Real.pm Sat Sep 7 18:56:52 2013 *************** *** 98,104 **** croak "vector dimensions do not match"; } ! sub clone { bless [@{$_[0]}] } sub set { &_check_dim; --- 98,104 ---- croak "vector dimensions do not match"; } ! sub clone { bless [@{$_[0]}], ref $_[0] } sub set { &_check_dim; *************** *** 109,115 **** sub add { &_check_dim; my ($v0, $v1) = @_; ! bless [map $v0->[$_] + $v1->[$_], 0..$#$v0] } sub add_me { --- 109,115 ---- sub add { &_check_dim; my ($v0, $v1) = @_; ! bless [map $v0->[$_] + $v1->[$_], 0..$#$v0], ref $_[ $_[2] ? 1 : 0]; } sub add_me { *************** *** 119,130 **** $v0; } ! sub neg { bless [map -$_, @{$_[0]}] } sub sub { &_check_dim; ! my ($v0, $v1) = ($_[2] ? @_[1, 0] : @_); ! bless [map $v0->[$_] - $v1->[$_], 0..$#$v0] } sub sub_me { --- 119,130 ---- $v0; } ! sub neg { bless [map -$_, @{$_[0]}], ref $_[0] } sub sub { &_check_dim; ! my ($v0, $v1 ) = ($_[2] ? @_[1, 0] : @_); ! bless [map $v0->[$_] - $v1->[$_], 0..$#$v0], ref $_[ $_[2] ? 1 : 0]; } sub sub_me { *************** *** 143,150 **** $acu; } else { ! my ($v, $s) = @_; ! bless [map $s * $_, @$v]; } } --- 143,150 ---- $acu; } else { ! my ($v, $s ) = @_; ! bless [map $s * $_, @$v], ref $_[0]; } } *************** *** 158,167 **** sub div { croak "can't use vector as dividend" if ($_[2] or ref $_[1]); ! my ($v, $div) = @_; $div == 0 and croak "illegal division by zero"; my $i = 1 / $div; ! bless [map $i * $_, @$v] } sub div_me { --- 158,167 ---- sub div { croak "can't use vector as dividend" if ($_[2] or ref $_[1]); ! my ($v, $div ) = @_; $div == 0 and croak "illegal division by zero"; my $i = 1 / $div; ! bless [map $i * $_, @$v], ref $_[0]; } sub div_me { *************** *** 193,199 **** if ($dim == 3) { return bless [$v0->[1] * $v1->[2] - $v0->[2] * $v1->[1], $v0->[2] * $v1->[0] - $v0->[0] * $v1->[2], ! $v0->[0] * $v1->[1] - $v0->[1] * $v1->[0]] } if ($dim == 7) { croak "cross product for dimension 7 not implemented yet, patches welcome!"; --- 193,200 ---- if ($dim == 3) { return bless [$v0->[1] * $v1->[2] - $v0->[2] * $v1->[1], $v0->[2] * $v1->[0] - $v0->[0] * $v1->[2], ! $v0->[0] * $v1->[1] - $v0->[1] * $v1->[0]], ! ref $_[0]; } if ($dim == 7) { croak "cross product for dimension 7 not implemented yet, patches welcome!"; *************** *** 287,293 **** $f += $_ * $_ for @$self; $f == 0 and croak "Illegal division by zero"; $f = 1/sqrt $f; ! bless [map $f * $_, @$self] } sub wrap { --- 288,294 ---- $f += $_ * $_ for @$self; $f == 0 and croak "Illegal division by zero"; $f = 1/sqrt $f; ! bless [map $f * $_, @$self], ref $self; } sub wrap { *************** *** 296,302 **** bless [map { my $s = $self->[$_]; my $c = $v->[$_]; ! $c - $s * POSIX::floor($c/$s) } (0..$#$self)]; } sub max_component { --- 297,304 ---- bless [map { my $s = $self->[$_]; my $c = $v->[$_]; ! $c - $s * POSIX::floor($c/$s) } (0..$#$self)], ! ref $self; } sub max_component { diff -c /dev/null 'Math-Vector-Real-0.10/t/subclass.t' Index: ./t/subclass.t *** ./t/subclass.t Wed Dec 31 19:00:00 1969 --- ./t/subclass.t Sat Sep 7 19:01:18 2013 *************** *** 0 **** --- 1,34 ---- + #!/usr/bin/perl + + use Test::More; + + use Math::Vector::Real; + + { + package SC; + use base 'Math::Vector::Real'; + } + + is( ref SC->new->clone, 'SC', 'clone' ); + + is( ref ( SC->new + V() ), 'SC', 'add' ); + + is( ref ( - SC->new ), 'SC', 'neg' ); + + is( ref ( SC->new * 3 ), 'SC', '* 3' ); + + is( ref ( 3 * SC->new ), 'SC', '3 *' ); + + is( ref ( SC->new / 3), 'SC', '/ 3 ' ); + + is( ref ( SC->new(1,1,1) x V(1,1,1) ), 'SC', ' x V' ); + + is( ref ( SC->new(1,1,1) x [1,1,1] ), 'SC', 'x []' ); + + is( ref ( [1,1,1] x SC->new(1,1,1) ), 'SC', '[] x' ); + + is( ref( SC->new(1,1,1)->versor ), 'SC', 'versor' ); + + is( ref( SC->new(1,1,1)->wrap( V(1,1,1) ) ), 'SC', 'wrap' ); + + done_testing; #### End of Patch data #### #### ApplyPatch data follows #### # Data version : 1.0 # Date generated : Sat Sep 7 21:14:52 2013 # Generated by : makepatch 2.05 # Recurse directories : Yes # Excluded files : (\A|/).*\~\Z # (\A|/).*\.a\Z # (\A|/).*\.bak\Z # (\A|/).*\.BAK\Z # (\A|/).*\.elc\Z # (\A|/).*\.exe\Z # (\A|/).*\.gz\Z # (\A|/).*\.ln\Z # (\A|/).*\.o\Z # (\A|/).*\.obj\Z # (\A|/).*\.olb\Z # (\A|/).*\.old\Z # (\A|/).*\.orig\Z # (\A|/).*\.rej\Z # (\A|/).*\.so\Z # (\A|/).*\.Z\Z # (\A|/)\.del\-.*\Z # (\A|/)\.make\.state\Z # (\A|/)\.nse_depinfo\Z # (\A|/)core\Z # (\A|/)tags\Z # (\A|/)TAGS\Z # p 'MANIFEST' 194 1378594914 0100644 # p 'lib/Math/Vector/Real.pm' 16956 1378594612 0100644 # c 't/subclass.t' 0 1378594878 0100664 #### End of ApplyPatch data #### #### End of Patch kit [created: Sat Sep 7 21:14:52 2013] #### #### Patch checksum: 251 6220 60227 #### #### Checksum: 281 7268 17977 ####
Math::Vector::Real was never intended to be extended via inheritance. You will have to describe me your use case in order to convince me that it is a good idea to do so.
On Mon Sep 09 04:50:30 2013, SALVA wrote: Show quoted text
> Math::Vector::Real was never intended to be extended via inheritance. > > You will have to describe me your use case in order to convince me > that it is a good idea to do so.
I've created a Vector class based upon Math::Vector::Real which interoperates with Math::Matrix and Math::Quaternion so that I can easily perform coordinate transformations. If the overloaded ops don't bless the results into my class, it breaks chaining of operations. For example, I would like to be able to do the following ($V1 - $V2) * ( ($M1 * $M2) * $Q ) Math::Vector::Real doesn't know how to left multiply matrices, so in order to do so, I need to override it's overloaded '*' operator. My overridden version of mul() understands how to handle 3x3 and 4x4 affine transformation matrices as well as Math::Quaternion objects. In the above exmaple, there's no functional need to override the '-' operator, but the result is blessed in Math::Vector::Real, not my Vector class, so the left multiplication of the matrices will fail. I've subclassed Math::Matrix (whose operators do bless into the subclass) so that it handles left multiplication of my Vector class and Math::Quaternion. So, ($M1 * $M2) * $V or ($M1 * $M2) * $Q will work. The ability to subclass Math::Vector::Real so that it can easily interoperate with other classes opens up a lot of functionality. If I can't do that, I essentially have to rewrite Math::Vector::Real, as the results of its overloaded operations erase the extra functionality in my subclass. I don't believe that the changes to make the overloaded operators aware of the subclass are too intrusive, or incur any great performance penalty (although I haven't benchmarked it). I have not looked at the XS version to see if its an onerous job to change its behavior similarly.
Subject: Re: [rt.cpan.org #88487] objects constructed via ops subs aren't blessed into parent object's class
Date: Tue, 10 Sep 2013 04:55:58 -0700 (PDT)
To: "bug-Math-Vector-Real [...] rt.cpan.org" <bug-Math-Vector-Real [...] rt.cpan.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Show quoted text
----- Original Message -----
> From: Diab Jerius via RT <bug-Math-Vector-Real@rt.cpan.org> > To: > Cc: > Sent: Tuesday, September 10, 2013 1:05 AM > Subject: [rt.cpan.org #88487] objects constructed via ops subs aren't blessed into parent object's class > >       Queue: Math-Vector-Real > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=88487 > > > On Mon Sep 09 04:50:30 2013, SALVA wrote:
>> Math::Vector::Real was never intended to be extended via inheritance. >> >> You will have to describe me your use case in order to convince me >> that it is a good idea to do so.
> > I've created a Vector class based upon Math::Vector::Real which > interoperates with Math::Matrix and Math::Quaternion so that I can > easily perform coordinate transformations. If the overloaded ops don't > bless the results into my class, it breaks chaining of operations. > > For example,  I would like to be able to do the following > >   ($V1 - $V2) * ( ($M1 * $M2) * $Q )
> Math::Vector::Real doesn't know how to left multiply matrices, so in > order to do so, I need to override it's overloaded '*' operator.  My > overridden version of mul() understands how to handle 3x3 and 4x4 > affine transformation matrices as well as Math::Quaternion objects. > > In the above exmaple, there's no functional need to override the '-' > operator, but the result is blessed in Math::Vector::Real, not my > Vector class, so the left multiplication of the matrices will fail. > > I've subclassed Math::Matrix (whose operators do bless into the > subclass) so that it handles left multiplication of my Vector class > and Math::Quaternion.  So, > >   ($M1 * $M2) * $V > > or > >   ($M1 * $M2) * $Q > > will work.
This is a problem I have already faced before and, IMO, inheritance is not the right solution. The right way to fix it would be to add support at the perl core for some kind of multimethod distpatching when both arguments to a binary overloadable operator have defined a method to handle it. In the meantime, a solution that works now, is to wrap in-place the overloading methods with others that do the dispatching and maybe some automatic type promotion. I have added a sample script to Math::Vector::Real distribution that shows how to make it inter-operate transparently with Math::Matrix:   https://github.com/salva/p5-Math-Vector-Real/blob/master/examples/Math-Matrix.pl In any case, is there any reason why you just don't use Math::Matrix for everything? Anything you do with vectors or quaternions can also be done with matrices.
On Tue Sep 10 07:56:35 2013, sfandino@yahoo.com wrote: Show quoted text
> This is a problem I have already faced before and, IMO, inheritance is > not the right solution. > > The right way to fix it would be to add support at the perl core for > some kind of multimethod distpatching when both arguments to a binary > overloadable operator have defined a method to handle it.
Yes. It's not likely this will happen anytime soon, however. Show quoted text
> > In the meantime, a solution that works now, is to wrap in-place the > overloading methods with others that do the dispatching and maybe some > automatic type promotion. > > I have added a sample script to Math::Vector::Real distribution that > shows how to make it inter-operate transparently with Math::Matrix: > >   https://github.com/salva/p5-Math-Vector- > Real/blob/master/examples/Math-Matrix.pl >
By overriding methods directly in the Math::Vector::Real class, there is the possibility that multiple competing overrides will clash. It creates a single point of contention and possible failure. The benefit of subclassing is that one doesn't have to worry about other modules overwriting my overrides Show quoted text
> In any case, is there any reason why you just don't use Math::Matrix > for everything?
Show quoted text
> Anything you do with vectors or quaternions can also be done with > matrices.
I have existing code which uses Math::Vector::Real, and it seemed natural to fold it into my new code.
Subject: Re: [rt.cpan.org #88487] objects constructed via ops subs aren't blessed into parent object's class
Date: Wed, 11 Sep 2013 00:57:34 -0700 (PDT)
To: "bug-Math-Vector-Real [...] rt.cpan.org" <bug-Math-Vector-Real [...] rt.cpan.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Show quoted text
----- Original Message -----
> From: Diab Jerius via RT <bug-Math-Vector-Real@rt.cpan.org> > To: > Cc: > Sent: Tuesday, September 10, 2013 5:59 PM > Subject: [rt.cpan.org #88487] objects constructed via ops subs aren't blessed into parent object's class > >       Queue: Math-Vector-Real > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=88487 > > > On Tue Sep 10 07:56:35 2013, sfandino@yahoo.com wrote:
>> This is a problem I have already faced before and, IMO, inheritance is >> not the right solution. >> >> The right way to fix it would be to add support at the perl core for >> some kind of multimethod distpatching when both arguments to a binary >> overloadable operator have defined a method to handle it.
> > Yes.  It's not likely this will happen anytime soon, however. > >
>> >> In the meantime, a solution that works now, is to wrap in-place the >> overloading methods with others that do the dispatching and maybe some >> automatic type promotion. >> >> I have added a sample script to Math::Vector::Real distribution that >> shows how to make it inter-operate transparently with Math::Matrix: >> >>   https://github.com/salva/p5-Math-Vector- >> Real/blob/master/examples/Math-Matrix.pl >>
> > By overriding methods directly in the Math::Vector::Real class, there > is the possibility that multiple competing overrides will clash. > It creates a single point of contention and possible failure.  The > benefit of subclassing is that one doesn't have to worry about other > modules overwriting my overrides
In theory yes, in practice this is very unlikely to happen. Anyway, there is no problem if the overloading subs are wrapped several times other than some performance degradation and then, if you really thing that would happen, you can always write a module to take care of the wrapping allowing cooperation of several modules.
>> In any case, is there any reason why you just don't use Math::Matrix >> for everything?
>
>> Anything you do with vectors or quaternions can also be done with >> matrices.
> > I have existing code which uses Math::Vector::Real, and it seemed natural > to fold it into my new code
Maybe an easy solution would be to create a pair of operators able to convert between Math::Vector::Real and Math::Matrix and to use them explicitly on the parts of the code where you combine both. For instance:   sub V2M { Math::Matrix->new([@{shift()}]) }   sub M2V { V(@{shift->[0]}) }   $V3 = M2V(V2M($V1 - $V2) * ($M1 * $M2) * $Q ));
Subject: Re: [rt.cpan.org #88487] objects constructed via ops subs aren't blessed into parent object's class
Date: Wed, 11 Sep 2013 09:25:48 -0400
To: bug-Math-Vector-Real [...] rt.cpan.org
From: Diab Jerius <djerius [...] cpan.org>
On Wed, Sep 11, 2013 at 3:57 AM, Salvador \"Fandiño\" via RT <bug-Math-Vector-Real@rt.cpan.org> wrote: Show quoted text
>> By overriding methods directly in the Math::Vector::Real class, there >> is the possibility that multiple competing overrides will clash. >> It creates a single point of contention and possible failure. The >> benefit of subclassing is that one doesn't have to worry about other >> modules overwriting my overrides
> > In theory yes, in practice this is very unlikely to happen.
I've discovered over the years that I no longer can predict what is likely to happen, in particular with software. I would rather ensure that it cannot happen. Show quoted text
> Anyway, there is no problem if the overloading subs are wrapped several times other than some performance degradation and then, if you really thing that would happen, you can always write a module to take care of the wrapping allowing cooperation of several modules.
Well, we'll have to agree to disagree on this. From my point of view, a few small changes to the module ensure that this sort of monkeypatching is not necessary. The amount and complexity of code required to ensure that all subsequent patching works seems an enormous technical debt (repeated by everyone who needs to do it) compared to the relatively non-invasive changes I suggest. You obviously have a different philosophy, which I respect, so I'm afraid we're at an impasse. Show quoted text
> Maybe an easy solution would be to create a pair of operators able to convert between Math::Vector::Real and Math::Matrix and to use them explicitly on the parts of the code where you combine both. > > For instance: > > sub V2M { Math::Matrix->new([@{shift()}]) } > sub M2V { V(@{shift->[0]}) } > > $V3 = M2V(V2M($V1 - $V2) * ($M1 * $M2) * $Q ));
This works, but unfortunately now exposes a more complex interface to users of my code. I'm trying for simplicity for the user, and this moves away from that goal. Diab
Subject: Re: [rt.cpan.org #88487] objects constructed via ops subs aren't blessed into parent object's class
Date: Wed, 11 Sep 2013 07:32:23 -0700 (PDT)
To: "bug-Math-Vector-Real [...] rt.cpan.org" <bug-Math-Vector-Real [...] rt.cpan.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Show quoted text
----- Original Message -----
> From: Diab Jerius via RT <bug-Math-Vector-Real@rt.cpan.org> > To: > Cc: > Sent: Wednesday, September 11, 2013 3:26 PM > Subject: Re: [rt.cpan.org #88487] objects constructed via ops subs aren't blessed into parent object's class > >       Queue: Math-Vector-Real > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=88487 > > > On Wed, Sep 11, 2013 at 3:57 AM, Salvador \"Fandiño\" via RT > <bug-Math-Vector-Real@rt.cpan.org> wrote:
>>> By overriding methods directly in the Math::Vector::Real class, there >>> is the possibility that multiple competing overrides will clash. >>> It creates a single point of contention and possible failure.  The >>> benefit of subclassing is that one doesn't have to worry about
> other
>>> modules overwriting my overrides
>> >> In theory yes, in practice this is very unlikely to happen.
> > I've discovered over the years that I no longer can predict what is > likely to happen, in particular with software.  I would rather ensure > that it cannot happen. >
>> Anyway, there is no problem if the overloading subs are wrapped several
> times other than some performance degradation and then, if you really thing that > would happen, you can always write a module to take care of the wrapping > allowing cooperation of several modules. > > Well, we'll have to agree to disagree on this.  From my point of view, > a few small changes to the module ensure that this sort of > monkeypatching is not necessary.  The amount and complexity of code > required to ensure that all subsequent patching works seems an > enormous technical debt (repeated by everyone who needs to do it) > compared to the relatively non-invasive changes I suggest.
I see two mayor problems with your changes: 1 - you are asking me to promote most of the module internal methods into the public API. 2 - supporting Math::Vector::Real usage as a base class would also mean to support that usage within the other modules on the Math::Vector::Real family So, I see them as quite intrusive and as, IMO, there are other better alternatives for doing what you need, I am not going to apply them. Sorry.
On Wed Sep 11 10:32:42 2013, sfandino@yahoo.com wrote: Show quoted text
> So, I see them as quite intrusive and as, IMO, there are other better > alternatives for doing what you need, I am not going to apply them. > Sorry.
Understood. Thanks for your effort in maintaining this package.