Opened 7 years ago
Closed 7 years ago
#17179 closed defect (fixed)
TIDES interface should convert exact parameters to floating points.
Reported by:  mmarco  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  interfaces  Keywords:  
Cc:  iMark  Merged in:  
Authors:  Miguel Marco  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  992c98b (Commits, GitHub, GitLab)  Commit:  992c98b54f579aa164c81f830385578eee6cd642 
Dependencies:  Stopgaps: 
Description
TIDES interface admits expressions like pi or 2^{(1/7) as parameters. They should be converted to floating point numbers of the appropiate precission. }
Change History (15)
comment:1 Changed 7 years ago by
 Type changed from PLEASE CHANGE to defect
comment:2 Changed 7 years ago by
Yes, sorry, i am working in a patch. I will include a branch in a few hours.
The idea is that, for instance, if you pass pi as an initial condition, the .c code created will have the string "pi" instead of "3.141592", and then the compiler would complain.
comment:3 Changed 7 years ago by
 Branch set to u/mmarco/ticket/17179
 Created changed from 10/19/14 18:31:21 to 10/19/14 18:31:21
 Modified changed from 10/19/14 19:46:39 to 10/19/14 19:46:39
comment:4 Changed 7 years ago by
 Commit set to fb97dfead7efb0375969dc048cf2fe8f752564e8
 Status changed from new to needs_review
comment:5 Changed 7 years ago by
 Status changed from needs_review to needs_work
Some comments:
 Instead of
mkdtemp()
, you can usetmp_dir()
fromsage.misc.temporary_file
. That gets cleaned automatically.
 Instead of
TIDES_RND
, I would use an explicitMPFR_RNDN
instead in thempfr_set_str()
calls.
 Remove the unused imports of
SAGE_ROOT
,os
,shutil
andN
.
 Do you really need
RR(SR(foo))
, isn'tRR(foo)
sufficient?
 Tides uses
int binary_precision(int prec) { return ceil(prec * 3.3219); }
to convert from decimal to binary precision, perhaps you should use the same formula?
 You should use the
.str(truncate=False)
method to convert aRealNumber
to a string, because by default Sage prints reals with less digits. Comparesage: RR(pi).str(truncate=False) '3.1415926535897931' sage: RR(pi).str() '3.14159265358979'
 Then something unrelated to this ticket:
exp()
in an expression currently doesn't work.
 Finally, fill in your real name as author on this ticket.
comment:6 followup: ↓ 8 Changed 7 years ago by
 Cc iMark added
RR(SR(foo))
is used because strings like "pi"
or "3/2"
are not accepted by the element constructor of real fields. They are accepted by the element constructor of SR though. So it was the best way i could find to create a RealField? element from these strings.
The real value of log(10)/log(2) is between 3.3219 and 3.322, so i prefeared to be conservative in this aspect.
This commit addresses mostly all your comments, but i think that the code could be greatly improved if we pass the domain to fast_callable, which would directly convert all constants, avoiding us the work of converting the exact expressions to the strings that represent them in floating point.
Besides, there is also some room for improvement in the performance of remove_repeated
.
I will work on that, but i am not sure if it should be done in this ticket (maybe we should change the title?)
comment:7 Changed 7 years ago by
 Commit changed from fb97dfead7efb0375969dc048cf2fe8f752564e8 to 5e6443ef83da23f5d5c911554b2e70ea0bc01bb3
Branch pushed to git repo; I updated commit sha1. New commits:
5e6443e  removed unnecessary imports, used tmp_dir and other minor fixes.

comment:8 in reply to: ↑ 6 Changed 7 years ago by
Replying to mmarco:
RR(SR(foo))
is used because strings like"pi"
or"3/2"
are not accepted by the element constructor of real fields.
I would say that's a good thing! Why should strings like that be accepted as input? I think it's reasonable to accept the elements pi
(element of SR
) and 3/2
(element of QQ
) but not the strings "pi"
or "3/2"
.
The real value of log(10)/log(2) is between 3.3219 and 3.322, so i prefeared to be conservative in this aspect.
If your precision doesn't agree with TIDES_PREC
, the result might actually be less precise: mpfr guarantees that the string representation (with truncate=False
in Sage) is such that the exact same real number will be created by mpfr_set_str(string, MPFR_RNDN)
provided that the precision is the same.
I understand that you prefer to round log(10)/log(2)
up, but then TIDES_PREC
should agree with this:
prec = ceil(dig * 3.322) RR = RealField(prec) ... outfile.write('\tTIDES_PREC = {};'.format(prec)) # instead of set_precision_digits()
This commit addresses mostly all your comments, but i think that the code could be greatly improved if we pass the domain to fast_callable, which would directly convert all constants, avoiding us the work of converting the exact expressions to the strings that represent them in floating point.
I have no experience with fast_callable
, so I cannot judge.
Besides, there is also some room for improvement in the performance of
remove_repeated
.I will work on that, but i am not sure if it should be done in this ticket
Not on this ticket.
comment:9 followup: ↓ 10 Changed 7 years ago by
You are right. I was trying to make an unnecessary extra conversion to string. This should address the issues for this ticket. I will open another one for the mentiones improvements in the future.
Just out of curiosity: do you actually use TIDES?
comment:10 in reply to: ↑ 9 Changed 7 years ago by
comment:11 Changed 7 years ago by
 Commit changed from 5e6443ef83da23f5d5c911554b2e70ea0bc01bb3 to 992c98b54f579aa164c81f830385578eee6cd642
Branch pushed to git repo; I updated commit sha1. New commits:
992c98b  Reverted RR(SR(foo)) trick, changed constant to match precission with TIDES

comment:12 Changed 7 years ago by
Is this supposed to be ready for review?
comment:14 Changed 7 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
Positive review, provided that tests pass.
comment:15 Changed 7 years ago by
 Branch changed from u/mmarco/ticket/17179 to 992c98b54f579aa164c81f830385578eee6cd642
 Resolution set to fixed
 Status changed from positive_review to closed
Could you please give an example of something which works/doesn't work?