Opened 6 years ago

Closed 6 years ago

#17179 closed defect (fixed)

TIDES interface should convert exact parameters to floating points.

Reported by: mmarco Owned by:
Priority: major Milestone: sage-6.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:

Status badges

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 6 years ago by jdemeyer

  • Type changed from PLEASE CHANGE to defect

Could you please give an example of something which works/doesn't work?

comment:2 Changed 6 years ago by mmarco

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 6 years ago by mmarco

  • 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 6 years ago by mmarco

  • Commit set to fb97dfead7efb0375969dc048cf2fe8f752564e8
  • Status changed from new to needs_review

This branch converts expressions like "pi" to their floating point literals.

This should take care of the issues in #16025


New commits:

fb97dfeHandle expressions like pi or 3/2 in tides interfaces

comment:5 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Some comments:

  • Instead of mkdtemp(), you can use tmp_dir() from sage.misc.temporary_file. That gets cleaned automatically.
  • Instead of TIDES_RND, I would use an explicit MPFR_RNDN instead in the mpfr_set_str() calls.
  • Remove the unused imports of SAGE_ROOT, os, shutil and N.
  • Do you really need RR(SR(foo)), isn't RR(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 a RealNumber to a string, because by default Sage prints reals with less digits. Compare
    sage: 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.
Last edited 6 years ago by jdemeyer (previous) (diff)

comment:6 follow-up: Changed 6 years ago by mmarco

  • Authors set to Miguel Marco
  • 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 6 years ago by git

  • Commit changed from fb97dfead7efb0375969dc048cf2fe8f752564e8 to 5e6443ef83da23f5d5c911554b2e70ea0bc01bb3

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

5e6443eremoved unnecessary imports, used tmp_dir and other minor fixes.

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

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 follow-up: Changed 6 years ago by mmarco

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 6 years ago by jdemeyer

Replying to mmarco:

Just out of curiosity: do you actually use TIDES?

No, I don't. I only posted a comment on the TIDES ticket because it didn't work anymore with #16025.

comment:11 Changed 6 years ago by git

  • Commit changed from 5e6443ef83da23f5d5c911554b2e70ea0bc01bb3 to 992c98b54f579aa164c81f830385578eee6cd642

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

992c98bReverted RR(SR(foo)) trick, changed constant to match precission with TIDES

comment:12 Changed 6 years ago by jdemeyer

Is this supposed to be ready for review?

comment:13 Changed 6 years ago by mmarco

  • Status changed from needs_work to needs_review

Yes, sorry

comment:14 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Positive review, provided that tests pass.

comment:15 Changed 6 years ago by vbraun

  • Branch changed from u/mmarco/ticket/17179 to 992c98b54f579aa164c81f830385578eee6cd642
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.