Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose C99/C++11 functions in printers #11637

Merged
merged 35 commits into from Mar 17, 2017
Merged

Conversation

bjodah
Copy link
Member

@bjodah bjodah commented Sep 20, 2016

Summary

TODO

  • handle standard kwarg in ccode.
  • move cfunctions to .util.codegen
  • write docstrings and update sphinx docs.
  • Add a C89CodePrinter class and make CCodePrinter inherit and extend it while being marked as deprecated.

Eventually we would like to have a helper function along the lines of:

>>> smart_ccode(2**x + log(x)/log(2))
'exp2(x) + log2(x)'
>>> smart_ccode((2*exp(x) - 2)*(3*exp(y) - 3), typically={x: And(-.1 < x, x < .1)})
'6*expm1(x)*(exp(y) - 1)'

but there is already a lot of things to discuss around this PR so I thought this was an appropriate time for a review.

Some bullet points for discussion:

  • how should we handle different standards? e.g. C89 vs. C99 vs. C11, e.g. now I created a subclass printer and set language='C99' standard='C99'
  • The helper functions in printing.cfunctions now define _eval_expand_func, _eval_rewrite_as_tractable and fdiff, anything more that would be useful here?
  • I introduced CXX98CodePrinter and CXX11CodePrinter in printing.cxxcode, using qualified names (std:: whenever possible). There is most certainly still work to be done here.
  • I would have liked to introduce a C89CodePrinter (which would have been the CCodePrinter minus some C99 features which have crept in), but I think that it would confuse more than anything else: people will wonder what role CCodePrinter has then.

@moorepants
Copy link
Member

If CCodePrinter has all of the syntax shared between versions of C and C++ would it by default be C89? Are recent versions a superset of C89 or are there API changes?

@bjodah
Copy link
Member Author

bjodah commented Sep 20, 2016

I would say that later standards are supersets, one exception that comes to mind is auto changing meaning in C++11, but currently it is not used by the printers.

@moorepants
Copy link
Member

If that is the case it seems fine to use CCodePrinter as a base class and create subclasses for each version and variation of the language. If the user uses CCodePrinter directly, they'll just get C89.

@asmeurer
Copy link
Member

Maybe the flag should be called standard, to match fcode.

@bjodah
Copy link
Member Author

bjodah commented Dec 20, 2016

@jhncls I added support for Min and Max to this PR, if you have the time, I'd be interested in your input on this.

Copy link
Member

@asmeurer asmeurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to put the functions in the codegen module.

@bjodah
Copy link
Member Author

bjodah commented Dec 21, 2016

The current version of the functions in cfunctions cannot be used as keys in dictionaries
I just added a failing test to demonstrate:

File ".../sympy/sympy/printing/tests/test_ccode.py", line 567, in test_get_math_macros
    assert macros[1/Sqrt(2)] == 'M_SQRT1_2'
KeyError: 1/Sqrt(2)

I would need to make the classes immutable, can we do so easily in SymPy? (I am only vaguely familiar with Basic and the metaclasses etc. that I think goes into that).

Or should I just avoid using such keys in dictionaries?

@asmeurer
Copy link
Member

Your test fails with ImportError. Hashing should work just fine since you've subclassed from Function.

@bjodah bjodah added PR: sympy's turn and removed PR: author's turn The PR has been reviewed and the author needs to submit more changes. labels Feb 28, 2017
@bjodah
Copy link
Member Author

bjodah commented Mar 13, 2017

@asmeurer I think this is ready for review. I've deprecated CCodePrinter and updated the codegen module accordingly.

@asmeurer
Copy link
Member

I'm not sure that I like that ccode always raises a deprecation warning now. Are we going to make the standard argument required?

This is our policy, by the way https://github.com/sympy/sympy/wiki/Deprecating-policy.

def _get_loop_opening_ending(self, indices):
open_lines = []
close_lines = []
loopstart = "for (int %(var)s=%(start)s; %(var)s<%(end)s; %(var)s++){" # C99
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only incompatibility between the current CCodePrinter and C89Printer? This doesn't seem like much, not particularly worth making ccode always give a deprecation warning.

Copy link
Member Author

@bjodah bjodah Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the only user visible change I think. asinh, acosh, atanh & tgamma are also C99 functions. But I haven't added print methods for those so they are still supported by the C89 codeprinter even though they shouldn't. Edit: Sorry looked in the wrong branch, I did remove those from the C89 printer (and erf too).

@asmeurer
Copy link
Member

I'm only worried about the deprecation warnings from ccode, which seem like they could be quite annoying. The rest looks fine. I admit I'm not very proficient in C++, so hopefully I haven't missed anything obvious, but I think if there are any mistakes here we won't catch them until we merge this and people start using it.

@bjodah
Copy link
Member Author

bjodah commented Mar 13, 2017

Regarding deprecation, I can definitely change back to not raising any deprecation warnings.
We could even make 'c99' the default for the standard kwarg of ccode and thereby only require users to specify standard if they want something else.

Regarding the C++ code, I think so too. I've used this branch myself for some things but I can't really say that I've tested it exhaustively. One alternative is that I mark the API as "provisional" for a version or two, i.e. we reserve the right to change it without a deprecation cycle?

@asmeurer
Copy link
Member

The specific output of the code printers does not count as API in my opinion. So we just have the new function cxxcode (which should probably be added to __init__.py), the codegen functions, and the code printer classes, which can be subclassed. Are there other things?

I don't know what the default standard should be. The advantage of c99 is that people who just use the function without realizing there is a standard option will see that it works for certain things that wouldn't work in c89. It does potentially break stuff for people using the old standard, but I don't see it as that major. I noticed that fcode defaults to 77, but perhaps C is more likely to be using the newer standards than Fortran or C++ (do you agree?).

@bjodah
Copy link
Member Author

bjodah commented Mar 13, 2017

I think those are the things (can't think of anything else).

It's hard to say what people in general use, I know that there has been discussions about using C11 as default in gcc. SymPy is a relatively young project. We should definitely support generating C89 code for e.g. people targeting embedded systems, but I think those user are fine with passing stadnard='c89' as a kwarg.

Should I try to remove the deprecation of CCodePrinter and just let it be an alias for C99CodePrinter? (and make the additions to __init__.py that you pointed out)

@asmeurer
Copy link
Member

We can keep the deprecation, but make C99CodePrinter the default in ccode. That way ccode doesn't raise a warning, but anyone subclassing from CCodePrinter will still see it. That was what I had originally meant by deprecating it, so that we can remove the class in the future.

@bjodah
Copy link
Member Author

bjodah commented Mar 16, 2017

@asmeurer so now there's considerably less deprecation warnings being raised. Good to merge?

assert ccode(expr, dereference=[z]) == "x + y + (*z) + sin((*z))"
with warnings.catch_warnings():
warnings.filterwarnings("ignore", category=SymPyDeprecationWarning)
assert ccode(expr, dereference=[z]) == "x + y + (*z) + sin((*z))"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this one still raise a warning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an oversight, I fixed it in the last commit.

@asmeurer
Copy link
Member

That looks a lot better.

@asmeurer asmeurer merged commit 50b5818 into sympy:master Mar 17, 2017
@asmeurer
Copy link
Member

Can you add something to the release notes for this https://github.com/sympy/sympy/wiki/Release-Notes-for-1.1

@bjodah
Copy link
Member Author

bjodah commented Mar 17, 2017

Thanks, I've updated the release notes.

@bjodah bjodah deleted the expm1-log1p branch March 17, 2017 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants