-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
reduction rule for m(nx)->(mn)x #1025
Conversation
lib/Parser/BOP/multiply.pm
Outdated
|
||
if ($reduce->{'m*(n*x)'} | ||
&& defined $self->{lop} | ||
&& defined $self->{rop}{lop} | ||
&& defined $self->{rop}{bop} | ||
&& $self->{lop}->class eq 'Number' | ||
&& $self->{rop}{lop}->class eq 'Number' | ||
&& $self->{rop}{bop} eq '*') | ||
{ | ||
my $m = $self->{lop}; | ||
$self->{lop} = $self->{rop}->copy; | ||
$self->{lop}->swapOps; | ||
$self->{lop}{lop} = $m; | ||
$self->{rop} = $self->{rop}{rop}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea, and one that certainly will help derivatives simplify. It is one of several that I had been meaning to add, but never got around to it.
There are couple of issues, here, though. First, the condition for the if-then is not as clean as it should be. First, you don't need to test that $self->{lop}
is defined, as that is always the case (the new
method always defines lop
, and if it isn't defined, something serious has gone wrong.
Then you should use $self->{rop}->isa('Parser::BOP::multiply')
to test if the right operator is a multiplication rather than testing if it has a bop
equal to *
.
Finally, manipulating the structure of the current object isn't advised. Rather, the process used by other reduction rules it so create a new sub-tree with the needed structure and return that. That makes sure that all the proper flags and such are properly set (e.g., the isConstant
marker, its type, and so on). For example, in this case, the rop
is likely not a constant, so its copy would not be marked constant, but when you adjust the copy's operands, they are now both constant, and $self->{lop}{isConstant}
should be true, but won't be. If you are in a context that has overridden some of the classes, then other other values may also be incorrectly set.
So I'd suggest changing this to something like
if ($reduce->{'m*(n*x)'}
&& $self->{lop}->class eq 'Number'
&& $self->{rop}->isa('Parser::BOP::multiply')
&& $self->{rop}{lop}->class eq 'Number'
) {
$self = $self->Item('BOP')->new(
$self->{equation}, self->{bop},
$self->Item('BOP')->new($self->{equation}, $self->{bop}, $self->{lop}, $self->{rop}{lop}),
$self->{rop}{rop}
)->reduce;
}
rather than manipulating the existing tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Davide! One question about all this (this morning after coffee); does it make sense to do it all right here, versus create a method for it? Like there is the swapOps
method. I started thinking there could be an associateOps
method and it could also be applied to m+(n+x)
. You could pass it something indicating if you want to regroup moving grouping to the left or to the right. If I created a method like that, does it belong in the same place as swapOps
is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along with m*(n*x)
and m+(n+x)
, there's also a need for (x+m)+n
. We already have x*n
and I think that removes any need for something like (x*m)*n
. Is there anything else along these lines to add? How do we feel about m*(x/n)
becoming m/n*x
? Or 1/x*y
becoming y/x
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to do it all right here, versus create a method for it?
If you end up needing this code in more than one place, then yes, a method would be useful, and it can go in BOP.pm like swapOps
.
it could also be applied to
m+(n+x)
Yes, but I suspect you might want a rule that does n+x => x+n
(where x is not a number), and then you can just looks for (x+n)+m
rather than all the the permutations (m+(x+n)
, m+(n+x)
, (n+x)+m
). You could also do (x-n)+m => (x+(m-n)
, (x+n)-m => x+(n-m)
, and (x-n)-m => x-(n+m)
.
I'm OK with the fraction reductions. Would you also want m*(n/x)=>(m*n)/x
and similar changes? There are lots of possibilities. You just have to be sure your don't have rules that undo other rules; e.g., negatives are factored out so that they can be combined via -(-x)=>x
or rules like x-(-y)=>x+y
, so you don't want a rule that puts negatives back in, as that could cause an infinite loop as the rules cycle the result.
I think you can removes the ->reduce
in my code above, as the only thing that can be reduced is the product m*n
and that will be taken care of in the new multiplication BOP automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n+x => x+n
makes me uneasy because people's polynomials like 1+x+x^2
that they intentionally arranged in that order will become x+1+x^2
. They could turn off the reduction rule, but now they'd have to edit their existing problems. Or this rule could be off by default, but that would be a new precedent (a reduction rule that is off by default). I'm not sure what to think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So would people prefer n+x => x+n
or x+n => n+x
? Without one of these, there will still be lots of unsimplified things. I could add both m+(n+x)
and (x+m)+n
, and without one of n+x => x+n
or x+n => n+x
, something like Formula("1+((2+x)+3)+4")
will still not reduce at all.
Maybe it should be set up so that both n+x => x+n
and x+n => n+x
are available, but of course both should not be used at the same time. But I'm not sure the best way to do that. The reduction rule could be called n+x
but then there could be a context flag that if set, makes n+x
behave like x+n
.
After this issue is sorted out, I'll think about the combinations involving a *
and /
; and those involving +
and -
(subtraction).
Moving this to the main thread. Would we prefer having a reduction Pushing additive constants left or right may be undesirable in some problems. I have thought about setting it up so that both |
Of course, that only happens if you call the Note that there are already several rules that will disrupt the order. For example, reducing It may be useful to collect together the rules that need to be disabled for polynomials into a variable, say $f = Compute("$c+${b}x+${a}x^2")->reduce(%Parser::PolynomialReductions); and not lose the ordering. You might even be able to introduce rules that would enforce the ordering, such as one that does In any case, it may be possible to produce a set of rules that causes polynomials to self-organize into a normalized form. There could be separate rules for highest power on the left and highest power on the right. Then you could have it would be possible to have a There are a lot of possibilities, here, so I would not be too worried about polynomials as I think they could be handled. It would be great to have reduction rules that improve the results for derivatives. |
I'm thinking about this more today, trying to wrap by head around it. If we have |
I pushed a commit that makes an abstract method for re-associating. And then it is used for the new reduction rule
I think if we go further like is discussed in the thread, it could be done carefully on a new PR. I'm not sure how to deal with the addiditive associativity issue I mentioned in the thread, and in the meantime we could at least have this first step. |
In case we want to go forward with this, I changed the target to PG-2.19. |
This adds a reduction rule
m*(n*x)
which will change things like2*(3*expression)
to(2*3)*expression
. I am not sure that this is the right way to do this, but it works with the testing I have done.I think this may address the issue that led to #887, but @drdrew42 should look at that. I think this may be the reduction rule that @dpvc mentioned there.
With:
prior to this commit, you will see something like
3*2x
and3*0.693147 2^x
. After this commit, you will see6x
and2.07944 2^x
.