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

reduction rule for m(nx)->(mn)x #1025

Merged
merged 2 commits into from
Aug 6, 2024
Merged

Conversation

Alex-Jordan
Copy link
Contributor

This adds a reduction rule m*(n*x) which will change things like 2*(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:

$f = Formula('3x^2');
$df = $f->D->reduce;
$g = Formula('3*2^x');
$dg = $g->D->reduce;

BEGIN_PGML
[`[$df]`]

[`[$dg]`]
END_PGML

prior to this commit, you will see something like 3*2x and 3*0.693147 2^x. After this commit, you will see 6x and 2.07944 2^x.

Comment on lines 71 to 85

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};
}
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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).

@Alex-Jordan
Copy link
Contributor Author

Moving this to the main thread.

Would we prefer having a reduction n+x => x+n or x+n => n+x? (Push constants right or push them left.) Without one of these, there will still be lots of unsimplified things. You could even have both associative reductions: 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.

Pushing additive constants left or right may be undesirable in some problems. I have thought about setting it 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. One idea is there could be a single reduction rule could be called n+x but there would also be a context flag that can makes n+x behave in the other direction, like x+n => n+x.

@dpvc
Copy link
Member

dpvc commented Mar 2, 2024

n+x => x+n makes me uneasy because people's polynomials like 1+x+x^2 that they intentionally arranged in that order.

Of course, that only happens if you call the reduce() method on them, but one might want to do that if there are potentially negative coefficients.

Note that there are already several rules that will disrupt the order. For example, reducing -2+5x+x^2 will produce 5x-2+x^2, and reducing -2-5x+x^2 will produce x^2-(2+5x). So one must already deal with these sorts of issues.

It may be useful to collect together the rules that need to be disabled for polynomials into a variable, say %Parser::PolynomialReductions so that you can do

$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 ax^n + bx^m => bx^m + ax^n when m < n, and (y + ax^n) + bx^m => (y + bx^m) + ax^n when m < n. You might also need ax^n + y => y + ax^n when y isn't of the form bx^m. A rule for ax^n + bx^n => (a+b)x^n would be useful, too. Of course, rules involving negatives would also be needed, but it might be sufficient to turn off the rules that factor out negatives, and add one that pushed a negative of a number into just the number being a negative one (that is, -(n) => (-n), since currently -n parses as a unary operator - applied to a positive number, if I recall correctly. Then you can get away with just the addition rules. But that may leave you with x + -5x^2. One would probably want a utility function that identifies a subtree as a monomial and returns the coefficient, base, and power, or undef if not a monomial. I'm just spitballing here, as I haven't tried these ideas out, and there could be hidden gotchas.

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 %Parser::PolynomialReduceAscending and %Parser:PolynomialReduceDescending or some such to activate the proper ruleset.

it would be possible to have a NormalizedPolynomial context that has only the needed rules (and no others), and subclass the BOP::add and other classes to implement the needed reductions so they are not checked for regular expression reductions, for efficiency. Then the rule sets could be %NormalizedPolynomal::Ascending and %NormalizedPolynomidal::Descending, for example.

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.

@Alex-Jordan
Copy link
Contributor Author

I'm thinking about this more today, trying to wrap by head around it. If we have n+x => x+n, then it seems we would also want n-x => -x+n, because the idea there is pushing constant terms to the right. But then we would enter an infinite loop from (-x)+y => y-x. What would we do to avoid that? Not have n-x => -x+n? Stop applying (-x)+y => y-x when y is a number?

@Alex-Jordan
Copy link
Contributor Author

I pushed a commit that makes an abstract method for re-associating. And then it is used for the new reduction rule m*(n*x). You can test the before and after results with:

$f = Formula('3x^2');
$df = $f->D->reduce;
$g = Formula('3*2^x');
$dg = $g->D->reduce;

BEGIN_PGML
[`[$df]`]

[`[$dg]`]
END_PGML

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.

@Alex-Jordan Alex-Jordan changed the base branch from develop to PG-2.19 July 16, 2024 17:33
@Alex-Jordan
Copy link
Contributor Author

In case we want to go forward with this, I changed the target to PG-2.19.

@drgrice1 drgrice1 merged commit 6b85f52 into openwebwork:PG-2.19 Aug 6, 2024
3 checks passed
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.

5 participants