Opened 4 years ago
Closed 4 years ago
#19351 closed enhancement (fixed)
Optimize initialization of RealIntervalFieldElement
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.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
 Dependencies set to #19330
comment:2 Changed 4 years ago by
 Branch set to u/jdemeyer/ticket/19351
comment:3 Changed 4 years ago by
 Commit set to 6e2b88bdcd57198ffeb97d4ed4575ee7833e5e0e
 Status changed from new to needs_review
comment:4 Changed 4 years ago by
 Commit changed from 6e2b88bdcd57198ffeb97d4ed4575ee7833e5e0e to 40ff5697119f0e9e425dabd195395ac18e41dbc1
Branch pushed to git repo; I updated commit sha1. New commits:
40ff569  Add conversion from RealDoubleElement and float

comment:5 followup: ↓ 6 Changed 4 years ago by
field.zero()
should be faster than field(0)
(not much though).
comment:6 in reply to: ↑ 5 Changed 4 years ago by
Replying to vdelecroix:
field.zero()
should be faster thanfield(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
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 followup: ↓ 12 Changed 4 years ago by
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
 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
 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:
842a66b  Fix a few details

comment:11 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:12 in reply to: ↑ 8 Changed 4 years ago by
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
 Branch changed from u/jdemeyer/ticket/19351 to 842a66b0b7f36f465d02d2f792d24408e61ba1d0
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Implement conversion of interval fields to real/complex fields
Use the new conversions in qqbar
Optimize initialization of RealIntervalFieldElement