Opened 4 years ago

Closed 4 years ago

#19351 closed enhancement (fixed)

Optimize initialization of RealIntervalFieldElement

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.9
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 842a66b (Commits) Commit: 842a66b0b7f36f465d02d2f792d24408e61ba1d0
Dependencies: #19330 Stopgaps:

Description

Profiling #19288 showed that this was a significant bottleneck.

Change History (13)

comment:1 Changed 4 years ago by jdemeyer

  • Dependencies set to #19330

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/19351

comment:3 Changed 4 years ago by jdemeyer

  • Commit set to 6e2b88bdcd57198ffeb97d4ed4575ee7833e5e0e
  • Status changed from new to needs_review

New commits:

048b036Implement conversion of interval fields to real/complex fields
752c401Use the new conversions in qqbar
6e2b88bOptimize initialization of RealIntervalFieldElement

comment:4 Changed 4 years ago by git

  • Commit changed from 6e2b88bdcd57198ffeb97d4ed4575ee7833e5e0e to 40ff5697119f0e9e425dabd195395ac18e41dbc1

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

40ff569Add conversion from RealDoubleElement and float

comment:5 follow-up: Changed 4 years ago by vdelecroix

field.zero() should be faster than field(0) (not much though).

comment:6 in reply to: ↑ 5 Changed 4 years ago by jdemeyer

Replying to vdelecroix:

field.zero() should be faster than field(0) (not much though).

I intentionally use field(0) because I need to mutate the number. field.zero() is probably cached and that would break things.

Ideally, I would use ComplexNumber.__new__() but that's not properly supported. The way I'm writing RealIntervalFieldElement.__cinit__() should be done for more element classes. But I didn't feel like rewriting also the ComplexNumber constructor here.

comment:7 Changed 4 years ago by mmezzarobba

Not a bug introduced here, but since you are cleaning up this code: I think

       elif isinstance(x, str):
           s = str(x).replace('..', ',').replace(' ','').replace('+infinity', '@inf@').replace('-infinity','-@inf@')
           if mpfi_set_str(self.value, s, base):
               raise TypeError("unable to convert {!r} to real interval".format(x))

should be

       elif isinstance(x, basestring):
           ...

or at least

       elif isinstance(x, (str, unicode)):
           ...

comment:8 follow-up: Changed 4 years ago by mmezzarobba

Also, do you know if this branch is really used anywhere?

           # try coercing to real
           try:
               ra = self._parent._lower_field()(x)
               rb = self._parent._upper_field()(x)

I'm far from convinced that conversions of sage objects to real fields with directed rounding really return upper/lower bounds...

comment:9 Changed 4 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

Positive review since the above comments are about peripheral issues, but feel free to make further changes if you agree with my comments.

comment:10 Changed 4 years ago by git

  • Commit changed from 40ff5697119f0e9e425dabd195395ac18e41dbc1 to 842a66b0b7f36f465d02d2f792d24408e61ba1d0
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

842a66bFix a few details

comment:11 Changed 4 years ago by mmezzarobba

  • Status changed from needs_review to positive_review

comment:12 in reply to: ↑ 8 Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

Also, do you know if this branch is really used anywhere?

It's used at least for constants, and it even works correctly:

sage: RIF(pi).endpoints()
(3.14159265358979, 3.14159265358980)
sage: RIF.pi().endpoints()
(3.14159265358979, 3.14159265358980)

comment:13 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/19351 to 842a66b0b7f36f465d02d2f792d24408e61ba1d0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.