Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21369 closed defect (fixed)

Update to pynac-0.6.9

Reported by: rws Owned by:
Priority: major Milestone: sage-7.4
Component: packages: standard Keywords:
Cc: embray, tscrim, fbissey Merged in:
Authors: Ralf Stephan, Erik Bray, Jeroen Demeyer Reviewers: Jeroen Demeyer, Ralf Stephan
Report Upstream: N/A Work issues:
Branch: b06cfa0 (Commits) Commit:
Dependencies: Stopgaps:

Description

Changelog:

  • fix bug affecting Cygwin compile (#21265)
  • combine some numerics in nested powers; (#21360)
  • fix and clean up ex::coefficients() (#20455)
  • expand() denominators too (#21302)
  • make ex::combine_fractions() recursive (#20858)
  • more normal() options (#21335)
  • Appell F1 function skeleton
  • updates from GiNaC

https://github.com/pynac/pynac/releases/download/pynac-0.6.9/pynac-0.6.9.tar.bz2

Change History (28)

comment:1 Changed 3 years ago by rws

  • Branch set to u/rws/update_to_pynac_0_6_9

comment:2 Changed 3 years ago by rws

  • Commit set to a4f58d734817e84d4ba5ba16cbd09587fa635f26
  • Status changed from new to needs_review

New commits:

b996f4fversion/chksum
91a08d2changes affecting Sage behaviour or interface
a4f58d7doctest fixes

comment:3 Changed 3 years ago by jdemeyer

This is a regression (based on a doctest you removed):

Before:

sage: k = GF(7); expand((k(1)*x^2 + k(1)*x)^7)
x^14 + x^7

After:

sage: k = GF(7); expand((k(1)*x^2 + k(1)*x)^7)
x^14 + 0*x^13 + 0*x^12 + 0*x^11 + 0*x^10 + 0*x^9 + 0*x^8 + x^7

comment:4 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_info

In general, for any doctest that you remove, it should be clearly justified why that doctest can be removed.

comment:5 follow-up: Changed 3 years ago by rws

Oh well, an impasse. My justification to me personally is that I do not want to fix a feature of symbolics that is better served by algebraic structures in Sage. As it is buggy already now, and we see no complaints, I also suspect that symbolic Mod is used by a negligible number of people.

I offer to start a sage-devel thread, and to work on a deprecation ticket in case others agree.

comment:6 Changed 3 years ago by git

  • Commit changed from a4f58d734817e84d4ba5ba16cbd09587fa635f26 to ac7d97159746aec5c68f25f1e628cd18c67ca609

Branch pushed to git repo; I updated commit sha1. New commits:

ac7d971revert removal of pos.char. doctests; add "known bug"

comment:7 follow-up: Changed 3 years ago by tscrim

  • Cc tscrim added

For # known bug, you should put in the correct answer.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by jdemeyer

Replying to tscrim:

For # known bug, you should put in the correct answer.

That is the case here if I'm not mistaken.

comment:9 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

Replying to rws:

I offer to start a sage-devel thread

Please do!

comment:10 in reply to: ↑ 8 Changed 3 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

For # known bug, you should put in the correct answer.

That is the case here if I'm not mistaken.

Ah, yes; sorry, I flopped comment:3.

comment:12 Changed 3 years ago by rws

  • Status changed from needs_info to needs_review

comment:13 Changed 3 years ago by jdemeyer

Thanks!

comment:14 follow-up: Changed 3 years ago by jdemeyer

  1. The changes to simplify_rational() seem more than just a Pynac update. For easier review, I suggest to move those changes to #21335 and focus on the upgrade.
  1. Why this?
    @@ -1387,10 +1387,19 @@ cdef class Expression(CommutativeRingElement):
                 ...
                 TypeError: unable to simplify to float approximation
             """
    +        from sage.functions.other import real, imag
             try:
    -            return float(self._eval_self(float))
    +            ret = float(self._eval_self(float))
             except TypeError:
    -            raise TypeError("unable to simplify to float approximation")
    +            try:
    +                c = (self._eval_self(complex))
    +                if imag(c) == 0:
    +                    ret = real(c)
    +                else:
    +                    raise
    +            except TypeError:
    +                raise TypeError("unable to simplify to float approximation")
    +        return ret
     
         def __complex__(self):
    

I understand and see that it makes sense, I am just wondering which bug it fixes.

  1. The code mentioned in 2. could be improved a bit. Instead of using the ret variable, I suggest just returning the value directly. In the complex case, I would use c = complex(self._eval_self(complex)) and then use the real and imag attributes (not functions).
  1. Can you justify
    • src/sage/symbolic/expression.pyx

      diff --git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx
      index 6c4171f..d780bbc 100644
      a b cdef class Expression(CommutativeRingElement): 
      11911191        except TypeError as err:
      11921192            # try the evaluation again with the complex field
      11931193            # corresponding to the parent R
      1194             if R is float:
       1194            if R in (float, complex):
      11951195                R_complex = complex
      11961196            else:
      11971197                try:

It seems pointless to have R_complex == R.

comment:15 Changed 3 years ago by git

  • Commit changed from ac7d97159746aec5c68f25f1e628cd18c67ca609 to 3dd8058f01117093516a52851e43ffb645b30c89

Branch pushed to git repo; I updated commit sha1. New commits:

3dd8058address reviewer's comments

comment:16 in reply to: ↑ 14 ; follow-ups: Changed 3 years ago by rws

Replying to jdemeyer:

  1. The changes to simplify_rational() seem more than just a Pynac update. For easier review, I suggest to move those changes to #21335 and focus on the upgrade.

OK.

  1. Why this?

...

I understand and see that it makes sense, I am just wondering which bug it fixes.

sage: float(sqrt(2)/sqrt(abs(-(I - 1)*sqrt(2) - I - 1)))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-9-5bba6e8d1a16> in <module>()
----> 1 float(sqrt(Integer(2))/sqrt(abs(-(I - Integer(1))*sqrt(Integer(2)) - I - Integer(1))))

/home/ralf/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.__float__ (build/cythonized/sage/symbolic/expression.cpp:10473)()
   1391             return float(self._eval_self(float))
   1392         except TypeError:
-> 1393             raise TypeError("unable to simplify to float approximation")
   1394
   1395     def __complex__(self):

TypeError: unable to simplify to float approximation

I was unable to find out why this bombed but it does so at least since Sage-7.1.

  1. Can you justify
    +            if R in (float, complex):
    

It seems pointless to have R_complex == R.

Yes but this also prevents the else branch done (R.complex_field() with R being complex).


New commits:

3dd8058address reviewer's comments

New commits:

3dd8058address reviewer's comments

comment:17 Changed 3 years ago by git

  • Commit changed from 3dd8058f01117093516a52851e43ffb645b30c89 to 1f29305b6ca0e176910ceeb18156081d3dface11

Branch pushed to git repo; I updated commit sha1. New commits:

1f29305add doctest

comment:18 in reply to: ↑ 16 Changed 3 years ago by jdemeyer

Replying to rws:

sage: float(sqrt(2)/sqrt(abs(-(I - 1)*sqrt(2) - I - 1)))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-9-5bba6e8d1a16> in <module>()
----> 1 float(sqrt(Integer(2))/sqrt(abs(-(I - Integer(1))*sqrt(Integer(2)) - I - Integer(1))))

/home/ralf/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.__float__ (build/cythonized/sage/symbolic/expression.cpp:10473)()
   1391             return float(self._eval_self(float))
   1392         except TypeError:
-> 1393             raise TypeError("unable to simplify to float approximation")
   1394
   1395     def __complex__(self):

TypeError: unable to simplify to float approximation

I was unable to find out why this bombed but it does so at least since Sage-7.1.

Can you add this as doctest then?

And delete the line from sage.functions.other import real, imag

comment:19 in reply to: ↑ 16 Changed 3 years ago by jdemeyer

Replying to rws:

Yes but this also prevents the else branch done (R.complex_field() with R being complex).

And why is that a problem? Sure, it will raise an AttributeError, but that's caught. So I still don't see the point.

comment:20 Changed 3 years ago by jdemeyer

I am happy with the branch on this ticket except for the float/complex stuff in expression.pyx that I need to look at more.

comment:21 Changed 3 years ago by git

  • Commit changed from 1f29305b6ca0e176910ceeb18156081d3dface11 to 0065f62b563eb6a7e67bd61fdba8d888a4d7e3f8

Branch pushed to git repo; I updated commit sha1. New commits:

0065f62address reviewer's comments

comment:22 Changed 3 years ago by jdemeyer

  • Branch changed from u/rws/update_to_pynac_0_6_9 to u/jdemeyer/update_to_pynac_0_6_9

comment:23 Changed 3 years ago by jdemeyer

  • Commit changed from 0065f62b563eb6a7e67bd61fdba8d888a4d7e3f8 to b06cfa068ae53d9bcb016a0bc4aba1ba21c8bc72
  • Reviewers set to Jeroen Demeyer

I fixed the underlying cause of the float/complex problem which is in _eval_self. This way, no changes to __float__ are needed. If you agree with this, you can set the ticket to positive_review.


New commits:

b06cfa0Fix _eval_self(float) for "real" complex expressions

comment:24 Changed 3 years ago by rws

  • Authors changed from Ralf Stephan, Erik M. Bray to Ralf Stephan, Erik M. Bray, Jeroen Demeyer
  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Ralf Stephan
  • Status changed from needs_review to positive_review

Is fine and passes tests. Thanks.

comment:25 Changed 3 years ago by tscrim

This also built without problems on Cygwin32/64 for me.

comment:26 Changed 3 years ago by fbissey

  • Cc fbissey added

comment:27 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/update_to_pynac_0_6_9 to b06cfa068ae53d9bcb016a0bc4aba1ba21c8bc72
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:28 Changed 3 years ago by chapoton

  • Authors changed from Ralf Stephan, Erik M. Bray, Jeroen Demeyer to Ralf Stephan, Erik Bray, Jeroen Demeyer
  • Commit b06cfa068ae53d9bcb016a0bc4aba1ba21c8bc72 deleted
Note: See TracTickets for help on using tickets.