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

assorted GLSL code printing improvements #19973

Merged
merged 9 commits into from Dec 31, 2020
Merged

Conversation

micahscopes
Copy link
Contributor

@micahscopes micahscopes commented Aug 17, 2020

Brief description of what is fixed or changed

I made some improvements to the GLSL printer, along with a small improvement to the generic codeprinter submodule.

In sympy.printing.codeprinter:

  • Allow custom multiline "spread" assignments when printing array-like objects, e.g.:
    >>> x_struct_members = symbols('x.a x.b x.c x.d')
    >>> print(glsl_code(Matrix([1,2,3,4]), assign_to=x_struct_members))
    x.a = 1;
    x.b = 2;
    x.c = 3;
    x.d = 4;

In sympy.printing.glsl:

  • allow passing in a array_type parameter, to support printing to various GLSL array types (e.g. float[N](...), int[N](...))
  • [bugfix] ensure that when operators are printed as functions, i.e. when 3 - x is printed as sub(3, x), that strictly negative valued expressions print correctly. For example: -x will now print as sub(0, x)
    • allow for a custom zero expression (e.g. zero()) to support printing expressions of abstract GLSL types.

Other comments

I've included tests and inline doctest examples!

Release Notes

  • printing
    • Allow spreading assignments across multiple symbols when printing multi-member objects.
    • Support various array constructor types when printing arrays to GLSL.
    • Fixes a bug when printing negative expressions to GLSL with operators printed as functions.
    • Support a custom 0 substitution for printing expressions representing various GLSL types.

@sympy-bot
Copy link

sympy-bot commented Aug 17, 2020

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • printing
    • Allow spreading assignments across multiple symbols when printing multi-member objects. (#19973 by @micahscopes)

    • Support various array constructor types when printing arrays to GLSL. (#19973 by @micahscopes)

    • Fixes a bug when printing negative expressions to GLSL with operators printed as functions. (#19973 by @micahscopes)

    • Support a custom 0 substitution for printing expressions representing various GLSL types. (#19973 by @micahscopes)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.8.

Click here to see the pull request description that was parsed.
#### Brief description of what is fixed or changed
I made some improvements to the GLSL printer, along with a small improvement to the generic `codeprinter` submodule.

In `sympy.printing.codeprinter`:
- Allow custom multiline "spread" assignments when printing array-like objects, e.g.:
```
    >>> x_struct_members = symbols('x.a x.b x.c x.d')
    >>> print(glsl_code(Matrix([1,2,3,4]), assign_to=x_struct_members))
    x.a = 1;
    x.b = 2;
    x.c = 3;
    x.d = 4;
```

In `sympy.printing.glsl`:
- allow passing in a `array_type` parameter, to support printing to various GLSL array types (e.g. `float[N](...)`, `int[N](...)`)
- [bugfix] ensure that when operators are printed as functions, i.e. when `3 - x` is printed as `sub(3, x)`, that strictly negative valued expressions print correctly.  For example: `-x` will now print as `sub(0, x)`
  - allow for a custom zero expression (e.g. `zero()`) to support printing expressions of abstract GLSL types.

#### Other comments
I've included tests and inline doctest examples!

#### Release Notes

<!-- BEGIN RELEASE NOTES -->
* printing
  * Allow spreading assignments across multiple symbols when printing multi-member objects.
  * Support various array constructor types when printing arrays to GLSL.
  * Fixes a bug when printing negative expressions to GLSL with operators printed as functions.
  * Support a custom `0` substitution for printing expressions representing various GLSL types.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@micahscopes
Copy link
Contributor Author

c.c. @bjodah @asmeurer

I made some improvements to the GLSL code printer, along with a little "spread assignment" feature for the generic code printer. Let me know if you have any questions!

@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #19973 (3fcc66f) into master (dd91882) will decrease coverage by 0.062%.
The diff coverage is 89.743%.

@@              Coverage Diff              @@
##            master    #19973       +/-   ##
=============================================
- Coverage   75.830%   75.767%   -0.063%     
=============================================
  Files          674       673        -1     
  Lines       175007    174513      -494     
  Branches     41336     41209      -127     
=============================================
- Hits        132709    132225      -484     
- Misses       36560     36571       +11     
+ Partials      5738      5717       -21     

Comment on lines 88 to 95
if isinstance(assign_to, str):
if expr.is_Matrix:
assign_to = MatrixSymbol(assign_to, *expr.shape)
else:
assign_to = Symbol(assign_to)
elif isinstance(assign_to, (list, tuple)):
assign_to = tuple(x if isinstance(x, Symbol) else Symbol(x) for x in assign_to)
elif not isinstance(assign_to, (Basic, type(None))):
raise TypeError("{0} cannot assign to object of type {1}".format(
type(self).__name__, type(assign_to)))
Copy link
Member

Choose a reason for hiding this comment

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

It strikes me as odd that the str -> symbol logic is not the same for lists and single elements. Perhaps something like:

def _handle_assign_to(expr, assign_to):
    if assign_to is None:
        return sympify(expr)
    if isinstance(assign_to, (list, tuple)):
        if len(expr) != len(assign_to):
            raise ValueError(...)
        return CodeBlock([_handle_assign_to(rhs, lhs) for lhs, rhs in zip(expr, assign_to)
    if isinstance(assign_to, str):
        assign_to = ... # as before
    elif not isinstance(assign_to, Basic):
        raise ... # as before
    return Assignment(assign_to, expr)

expr = _handle_assign_to(expr, assign_to)

Which would maybe support nested tuple targets too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah I think what you're suggesting makes sense. I'll look into it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! Nice suggestion, I implemented this in the last commit here. Basic nested tuple/list assignments work just as you'd imagined 🥳. It was simplest to make a test case in the test_glsl.py file, since codeprinter.py is pretty abstract and doesn't implement all the basic printing functions. Hopefully that suffices.

Here's the working nested tuple assignment example:

    expr = ((1,2,3), (1,2,3))
    assign_to = (symbols('a b c'), symbols('x y z'))
    glsl_code(expr, assign_to = assign_to)

returns

a = 1;
b = 2;
c = 3;
x = 1;
y = 2;
z = 3;

sympy/printing/glsl.py Outdated Show resolved Hide resolved
sympy/printing/glsl.py Outdated Show resolved Hide resolved
@oscarbenjamin
Copy link
Contributor

It looks like there are some useful changes here but I see that there are unaddressed comments as well.

@micahscopes what's the status of this? Are you still working on it?

@micahscopes
Copy link
Contributor Author

@oscarbenjamin I haven't made time to give this a good look since @eric-wieser left those helpful comments. Hopefully soon!

@czgdp1807
Copy link
Member

@micahscopes Would you like to conclude this PR? Please address the comments if so, otherwise, do not close this PR. Thanks for your contributions.

@micahscopes
Copy link
Contributor Author

@micahscopes Would you like to conclude this PR? Please address the comments if so, otherwise, do not close this PR. Thanks for your contributions.

I finally got around to addressing all of @eric-wieser's comments, so I think this should be just about finished up.

sympy/printing/glsl.py Outdated Show resolved Hide resolved
@micahscopes
Copy link
Contributor Author

@eric-wieser should be good now

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

As discussed offline, this looks good - would be great to see a test for the exceptions, but if you don't get around to it soon, then better to merge it anyway than do nothing.

@micahscopes
Copy link
Contributor Author

As discussed offline, this looks good - would be great to see a test for the exceptions, but if you don't get around to it soon, then better to merge it anyway than do nothing.

I added a test for the mismatched length exception. I think this is quite ready to merge now. Thanks again for the very thorough review @eric-wieser!

@micahscopes
Copy link
Contributor Author

@czgdp1807 this should be ready to go!

@czgdp1807
Copy link
Member

Will merge tomorrow if no objections raised and if it doesn't get merged until then.

@eric-wieser
Copy link
Member

Travis seems unwilling to schedule a build. Is sympy out of travis credits?

@oscarbenjamin
Copy link
Contributor

We are migrating to Github Actions from Travis: #20374

Travis has significantly reduced the number of workers available to open source projects at least on their current domain. The result is a backlog of jobs which varies on a daily/weekly basis but seems to be particularly high today:
https://www.traviscistatus.com/#month

@oscarbenjamin
Copy link
Contributor

Closing and reopening to restart the tests

@micahscopes
Copy link
Contributor Author

Closing and re-opening to (hopefully) kick off the Travis build again...

@micahscopes micahscopes reopened this Dec 15, 2020
@micahscopes
Copy link
Contributor Author

@czgdp1807 this is ready to go 👍

@czgdp1807
Copy link
Member

@eric-wieser Would you like to suggest any further changes in this PR? There are some new commits after your approval.

@micahscopes
Copy link
Contributor Author

@czgdp1807 I think for the most part, those commits showing since the approval were the result of my force push after rebasing (to get rid of Travis configuration so that the CI would pass).

There was one substantive change since the approval, but it was one I'd discussed with @eric-wieser (an additional test).

@czgdp1807 czgdp1807 merged commit 4566a7d into sympy:master Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants