Opened 3 years ago

Last modified 2 years ago

#21274 needs_work enhancement

frac(x) immediate simplifications

Reported by: rws Owned by:
Priority: minor Milestone: sage-7.4
Component: symbolics Keywords:
Cc: mkoeppe Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/frac_x__immediate_simplifications (Commits) Commit: bf362867b8089a8a68b4e51ac67582cfe63b4679
Dependencies: Stopgaps:

Description

As a followup to #21232 these automatic simplifications should be added:

sage: frac(frac(x))
frac(x)
sage: frac(floor(x))  # ceil
0
sage: assume(x, 'integer')
sage: frac(x)
0
sage: assume(x > 0)
sage: frac(tanh(x))
tanh(x)

Change History (11)

comment:1 Changed 3 years ago by rws

  • Branch set to u/rws/frac_x__immediate_simplifications

comment:2 Changed 3 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to bf362867b8089a8a68b4e51ac67582cfe63b4679
  • Status changed from new to needs_review

New commits:

bf3628621274: frac(x) immediate simplifications

comment:3 Changed 3 years ago by mkoeppe

This all seems to work, but it's handling very specific cases and it would be much nicer if the system knew this:

sage: floor(x).is_integer()
True     # wishful

comment:4 Changed 3 years ago by rws

Actually the mechanics for this exists (eg https://github.com/pynac/pynac/issues/78) but floor, as it is not a GinacFunction, needs a different approach in Pynac than the ones implemented in that issue. Best I think is to first review #12121 and then after merge make floor a GinacFunction.

comment:5 Changed 3 years ago by tscrim

A very micro optimization:

sage: %timeit ZZ.zero()
The slowest run took 46.49 times longer than the fastest. This could mean that an intermediate result is being cached.
10000000 loops, best of 3: 61.5 ns per loop
sage: %timeit 0    # Preparsed as Integer(0) with the 0 being a python int
The slowest run took 40.29 times longer than the fastest. This could mean that an intermediate result is being cached.
10000000 loops, best of 3: 71 ns per loop
sage: z = int(0)
sage: %timeit Integer(z)
The slowest run took 53.93 times longer than the fastest. This could mean that an intermediate result is being cached.
10000000 loops, best of 3: 75.2 ns per loop

comment:6 Changed 3 years ago by vdelecroix

Why is this part of the function frac? The function should not care about simplifying the result in any way. If you want a custom frac you would better consider having a frac method to symbolic expression (and having the function frac calling it).

comment:7 Changed 3 years ago by rws

Very interesting. I completely forgot about these. First, if you look at functions/* you'll note a lot of expression arguments handled by custom code in eval() member functions, so called special values. We should write a Expression member function for all these functions? That would be an even larger class than it is now.

Secondly, I removed the Expression.sin() member and the only doctest that fails is the one explicitly calling it. So, it seems to me, other than taste or maybe convenience, there is no reason to have it. Didn't Occam demand not to multiply symbols beyond necessity?

I'm aware of the function eval() member being too general for expressions, and I'm constantly moving code into Pynac, so the special handling in eval() should be seen as a first step for testing code that is well maintainable because it's Python. If you insist I'll immediately move the frac code to C++. But please don't enlarge Expression any further and help with efforts to actually make it smaller.

Last edited 3 years ago by rws (previous) (diff)

comment:8 Changed 3 years ago by rws

As a further data point SymPy does not have functions as expression members at all, not even .square() or .abs().

comment:9 follow-up: Changed 3 years ago by vdelecroix

The main difference with SymPy is that Sage deals with a lot of different objects. sqrt can be called on integers, rationals, polynomials, power series, ... Not only symbolic expression. You really want to minimize type checking in the function to minimize the overhead (see e.g. the kind of discussions we had in #11332).

comment:10 Changed 3 years ago by rws

  • Status changed from needs_review to needs_work

comment:11 in reply to: ↑ 9 Changed 2 years ago by rws

Replying to vdelecroix:

... You really want to minimize type checking in the function to minimize the overhead (see e.g. the kind of discussions we had in #11332).

More and more I agree with this, see #24178. Still, I really don't want the symbolic version as Expression member but as standalone symbolic function, i.e., in sage/functions.

Note: See TracTickets for help on using tickets.