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

bug with derivative operator in Fraction context #1168

Open
Alex-Jordan opened this issue Jan 7, 2025 · 15 comments
Open

bug with derivative operator in Fraction context #1168

Alex-Jordan opened this issue Jan 7, 2025 · 15 comments

Comments

@Alex-Jordan
Copy link
Contributor

On develop, here is a MWE of something that causes an error, but it does not cause an error in 2.19:

DOCUMENT();

loadMacros(qw(PGstandard.pl MathObjects.pl contextFraction.pl));
Context("Fraction")->flags->set(reduceConstants=>0);
$f = Formula('1/2x')->D('x');

ENDDOCUMENT();

I boiled this down from a complicated exercise that was reported broken since I moved our server to develop. I can avoid the error by not setting reduceConstants=>0. However doing so causes some other issue (cosmetic issues, not PG errors).

@dpvc
Copy link
Member

dpvc commented Jan 7, 2025

Try applying this patch to the contextExtensions.pl file. If that seems o work for you, I can make a PR for it and explain what is going on.

diff --git a/macros/contexts/contextExtensions.pl b/macros/contexts/contextExtensions.pl
index 72833110..13fd075c 100644
--- a/macros/contexts/contextExtensions.pl
+++ b/macros/contexts/contextExtensions.pl
@@ -449,6 +449,19 @@ sub extensionContext {
        return $class;
 }
 
+#
+#  Trap calls to methods in the super-class that aren't overridden in the subclass
+#  and perform the super-class method with the given arguments.
+#
+sub AUTOLOAD {
+       my $self = shift;
+       my $name = $AUTOLOAD;
+       $name =~ s/.*:://;
+       my $meth = $self->super($name);
+       return &$meth($self, @_) if ref($meth) eq 'CODE';
+       die "Undefined method $name for " . ref($self);
+}
+

@somiaj
Copy link
Contributor

somiaj commented Jan 7, 2025

@dpvc I tested your patch, and it does fix the original issue with the D method not existing and the derivative is computed correctly. But I get a lot of warnings when testing the problem in the problem editor.

Warning messages

    (in cleanup) Undefined method DESTROY for context::Fraction::BOP::divide at line 462 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::AUTOLOAD called at line 86 of [PG]/lib/Parser/BOP.pm
    from within Parser::BOP::reduce called at line 84 of [PG]/lib/Parser/Differentiation.pm
    from within Parser::BOP::multiply::D called at line 40 of [PG]/lib/Parser/Differentiation.pm
    from within Parser::D called at line 5 of undefined
    (in cleanup) Undefined method DESTROY for context::Fraction::Parser::Value at line 462 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::AUTOLOAD called at line 84 of [PG]/lib/Parser/Differentiation.pm
    from within Parser::BOP::multiply::D called at line 40 of [PG]/lib/Parser/Differentiation.pm
    from within Parser::D called at line 5 of undefined
    (in cleanup) Undefined method DESTROY for context::Fraction::Parser::Value at line 462 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::AUTOLOAD called at line 42 of [PG]/lib/Parser/Differentiation.pm
    from within Parser::D called at line 5 of undefined
    (in cleanup) Can't use an undefined value as a HASH reference at line 394 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::superClass called at line 384 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::super called at line 460 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::AUTOLOAD called at line 7 of undefined

@Alex-Jordan
Copy link
Contributor Author

Thanks @dpvc. I'm seeing the same as @somiaj, with warnings about DESTROY.

@dpvc
Copy link
Member

dpvc commented Jan 7, 2025

OK, thanks. I'll have to look into it further. I was afraid there might be unwanted side effects. I may be able to make a smarter function for handling it,.

@Alex-Jordan
Copy link
Contributor Author

I added sub DESTROY {}; immediately under your patch and that at least clears up the warnings. Unsure if this was wise. At least one other sub DESTROY in the repo is empty like this.

@dpvc
Copy link
Member

dpvc commented Jan 7, 2025

Try changing the AUTOLOAD function to

#
#  Trap calls to methods that aren't overridden in the subclass
#  and perform the super-class method with the given arguments.
#
sub AUTOLOAD {
	my $self = @_[0];
	my $name = $AUTOLOAD;
	$name =~ s/.*:://;
	my $meth = $self->super($name);
	goto &$meth if ref($meth) eq 'CODE';
}

and see if that is better.

@somiaj
Copy link
Contributor

somiaj commented Jan 7, 2025

I too can confirm that adding sub DESTROY {} removed the warnings.

@somiaj
Copy link
Contributor

somiaj commented Jan 7, 2025

@dpvc Your recent suggestion didn't stop the warnings, I still see them. In addition I get the following Scalar value @_[0] better written as $_[0]. Would the first line as my ($self) = @_; be better there?

@dpvc
Copy link
Member

dpvc commented Jan 7, 2025

OOPS, yes, $_[0] is correct, but your version is also OK.

I don't see how you can still be getting the message because the line that produced is has been removed. Are you sure the correct macro file is loading? Or are they just similar messages? The message

Undefined method DESTROY for context::Fraction::BOP::divide at line 462 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::AUTOLOAD called at line 86 of [PG]/lib/Parser/BOP.pm

was generated by the die command that should have been removed in the new version of AUTOLOAD.

@somiaj
Copy link
Contributor

somiaj commented Jan 7, 2025

I didn't look at the warnings that closely so yes, they are different. Here they are.

 
    (in cleanup) Can't use an undefined value as a HASH reference at line 394 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::superClass called at line 384 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::super called at line 460 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::AUTOLOAD called at line 7 of undefined
    (in cleanup) Can't use an undefined value as a HASH reference at line 394 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::superClass called at line 384 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::super called at line 460 of [PG]/macros/contexts/contextExtensions.pl
    from within context::Extensions::Super::AUTOLOAD called at line 7 of undefined

@dpvc
Copy link
Member

dpvc commented Jan 7, 2025

OK, here is another try:

sub AUTOLOAD {
	my ($self) = @_;
	my $name = $AUTOLOAD;
	$name =~ s/.*:://;
	my $meth = $self->context ? $self->super($name) : undef;
	goto &$meth if ref($meth) eq 'CODE';
}

@somiaj
Copy link
Contributor

somiaj commented Jan 7, 2025

That appears to fix all the warnings for me. Thanks.

@Alex-Jordan
Copy link
Contributor Author

The latest is working for me, and the original exercise that led to this report is working as expected now. Thanks!

@dpvc
Copy link
Member

dpvc commented Jan 7, 2025

I think this may be a slightly better solution is

diff --git a/macros/contexts/contextExtensions.pl b/macros/contexts/contextExtensions.pl
index 72833110..c56f065e 100644
--- a/macros/contexts/contextExtensions.pl
+++ b/macros/contexts/contextExtensions.pl
@@ -423,6 +423,10 @@ sub new {
        return &{ $self->super("new", $context) }($self, @_);
 }
 
+sub context {
+       return Value::context(@_);
+}
+
 #
 #  Get the object's class from its class name
 #
@@ -449,6 +453,18 @@ sub extensionContext {
        return $class;
 }
 
+#
+#  Trap calls to methods that aren't overridden in the subclass
+#  and perform the super-class method with the given arguments.
+#
+sub AUTOLOAD {
+       my ($self) = @_;
+       my $name = $AUTOLOAD;
+       $name =~ s/.*:://;
+       my $meth = $self->super($name);
+       goto &$meth if ref($meth) eq 'CODE';
+}
+
 #################################################################################################
 #################################################################################################

as this avoids a potential infinite loop with the context method (which is used internally in the AUTOLOAD function, so could be problematic, and was so in some tests that I was trying).

@dpvc
Copy link
Member

dpvc commented Jan 8, 2025

I have reconsidered my AUTOLOAD solution, as I was thinking incorrectly about how the contextExtensions.pl now works. Originally, the overridden classes had the original classes as subclasses, and I was forgetting that that approach had changed. The current extension mechanism is to save the original class name and override it with a new class that calls the original class's new method to create itself, but overrides the _check() method to check if it represent a fraction (or whatever the extension is implementing). If it doesn't, the object is mutated back to the original class, and the overriden class is no longer in use, but if it does represent the fraction, or whatever, then the new class stays in effect and implements the new behavior. It does not have the original as a subclass, although it can call the original class methods if it explicitly wants to.

That makes the new classes not really subclasses or the original, but a kind of "pass through" to the original when they don't come into play.

So that means we don't want the AUTOLOAD method after all, since we aren't a subclass of the original. instead, we need to implement the appropriate functionality for the fraction or other object that is replacing the original definitions.

In this case, that means we need to implement the D() method for when the division sign is for the fraction class. I missed this originally because the D() methods for all classes are added to the Parser classes in a separate Deferentiaion.pl file rather than in the Parser class files themselves, and I didn't think of it.

I have made a PR for what I think is the correct solution to this problem, so I would remove the changes I suggested above, and try this new PR instead.

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

No branches or pull requests

3 participants