Opened 5 years ago
Closed 5 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 5 years ago by
- Dependencies set to #19330
comment:2 Changed 5 years ago by
- Branch set to u/jdemeyer/ticket/19351
comment:3 Changed 5 years ago by
- Commit set to 6e2b88bdcd57198ffeb97d4ed4575ee7833e5e0e
- Status changed from new to needs_review
comment:4 Changed 5 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 follow-up: ↓ 6 Changed 5 years ago by
field.zero()
should be faster than field(0)
(not much though).
comment:6 in reply to: ↑ 5 Changed 5 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 5 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 follow-up: ↓ 12 Changed 5 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 5 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 5 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 5 years ago by
- Status changed from needs_review to positive_review
comment:12 in reply to: ↑ 8 Changed 5 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 5 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