New MatchPyCpp package in utilities

[Upabjojr]

I think symengine/utilities is the optimal location for this package. A Python file is included, which acts as code generator for the SymEngine.

• move SymEnginePrinter to SymPy.
• optional wildcards testing.
• sequence-matching wildcard ==> not urgent, open separate issue
• check var.find(...) == var.end() for optimizations.
• add more const ... & to function parameters.
• add typedef map<string, RCP<const Basic>> SubstitutionBasic ? ==> open separate issue

[certik]

@Upabjojr ping me once this is ready for review.

[Upabjojr]

Latest updates:

• no more coroutines/generators, they are all expanded to vectors and returned in full.
• pattern matching requiring the commutative property is still not working.

Reasons for discarding the attempts at porting generators to C++:

• Library coroutine.h messes up the reference counter of both Teuchos and STL.
• Using setjmp gives similar problems.
• Using the block trick is too complicated and is likely to make the code hard for support.
• It looks like clang and Microsoft Visual C++ now support an experimental co_yield keyword, similar to Python's yield. It's still experimental, but hopefully it will get into the C++ standard soon.

[asmeurer]

Don't patterns return a combinatorial number of matches in general? For instance matching a*b*c against x*y*z matches in 3! ways (even more if you allow things like a=1,b=x,c=y*z). I think you need some way to return one match at a time.

[Upabjojr]

Yes, I agree. But I'm getting so much headache implementing coroutines in C++ that I'm giving up for now.

If really needed, we can try multithreading, which is supported by the C++ standard. I fear that even multithreading may cause disturbances to the reference counter.

I just wanna make the commutative matcher work, otherwise this project will never end. Optimizations will be done in the future.

[Upabjojr]

The issues with the reference counter are quite serious, sometimes the memory is freed too early, resulting in segmentation faults during the execution.

[certik]

I agree, let's get something working, then work on optimizations. Regarding generators in C++, let's do a video call later, so that I can better understand what you are trying to do. In general I would prefer not to use the very latest features, especially if they are not even in the standard yet --- usually there is a way to express the same functionality using older C++. But as you said, we can work on this later, looks like you have a workaround now.

[isuruf]

@Upabjojr, do you have a branch with coroutine.h that shows the reference counter issues? If so, I can have a look while you are working on the branch without generators.

[Upabjojr]

@Upabjojr, do you have a branch with coroutine.h that shows the reference counter issues? If so, I can have a look while you are working on the branch without generators.

Yes, in #1482

There are various example files:

1. example_coroutine.cpp
2. example_coroutine2.cpp
3. example_setjmp.cpp

In the first two I'm using the coroutine.h header, as suggested by you. I get errors about memory not having been freed when using gdb.

Example 3 uses setjmp, there are also problems.

[isuruf]

Thanks @Upabjojr, I do see memory leaks when using coroutine.h, but not when using boost::coroutines. coroutine.h must be doing something wrong here.

[Upabjojr]

So maybe we should use the boost library.

[isuruf]

Hmm, boost.coroutine is now deprecated in favour of boost.coroutine2, but with boost.coroutine2 there are memory leaks.

[isuruf]

Looks like when the stack frame is deleted, the destructors are not called for the C++ objects unless C++20 coroutines are used.

[isuruf]

Code generation will have to avoid temporaries or be declared in the match_root class if anything other than C++20 coroutines are used.

[Upabjojr]

Coroutines have not yet been approved for C++20. I hope they will.

[isuruf]

Pushed a commit to #1482 to make example_coroutine2.cpp work properly. No more memleaks

[Upabjojr]

OK thanks.

[Upabjojr]

I got to fix the tests. The Python files still untested. Testing them would require installing a whole Python environment with SymPy and MatchPy dependencies. Maybe these files should eventually be moved to SymPy.

Anyway, I guess it's time to prepare for merging.

[isuruf]

Testing them would require installing a whole Python environment with SymPy and MatchPy dependencies

We are using conda to install the C++ dependencies, so it's not hard to install a python environment. Can you create an environment.yml file in matchpygen directory with all the packages you need?

[Upabjojr]

Ok I can try.

[Upabjojr]

OK, so now the tests run generate_tests.py and check for git diff not to show any difference. Is that correct?

If that's correct, I believe we can merge this branch.

[certik]

I am checking that the tests fail in #1515.

[certik]

It seems to work now, the tests pass and fail properly. I am +1 to merge this after all tests pass.

[Upabjojr]

OK, let's merge so that we can close the NumFOCUS project.

[Upabjojr]

@certik is this PR going to be merged?

[certik]

@Upabjojr sorry, I was waiting for the tests to pass, but forgot about it. I wish there was a notification for that.

[isuruf]

There's https://mergify.io/ where you can set a label merge-when-green on a PR and a bot will merge when the CI passes.

[asmeurer]

We also considered adding a feature like that to the SymPy bot. The problem is that then the merge commit comes from the bot, instead of the person who merged it. I wish GitHub would just implement this natively, so you could click "merge when the tests pass" and as long as the tests don't fail, new commits aren't pushed, or someone else doesn't cancel it, it will automerge once all the tests pass.

[jondo]

FWIW, this auto-merge feature exists in GitLab.

[isuruf]

Mergify can comment on the PR instead of merging it. Also the SymPy bot could merge with the author's name and email id, but the bot needs to have a way to associate github handle with an email.

[asmeurer]

That would require disabling branch protection. Right now no one can push directly to master. The branch protection forces every commit to come from a merged pull request (with passing status).

[certik]

@jondo yes, I've been using GitLab for new projects for a few years precisely because of little things like this. But I don't want to move SymPy or SymEngine, as the pain of moving doesn't seem worth the benefits (yet). Plus GitLab has some things that seem a bit worse, like searching for projects, smaller community, etc.

[asmeurer]

I've included it in my blog post of things I'd like to see GitHub improve (number 15). I've gotten responses from people at GitHub so I know they've seen it. Let's see how many things they implement. GitHub seems to be doing better with this sort of thing since they got acquired by Microsoft and got a new CEO (at least now they are more. I'm curious how responsive GitLab is to these sorts of requests, relative to GitHub.

[certik]

@asmeurer, GitLab has these feature requests as public issues, so you can see the progress there. Anybody can help implementing the features. GitLab itself is usually very slow on things that are not part of their roadmap, but eventually it does seem to come around. Here is an example of a long standing issue in GitLab (something that GitHub had for a long time): https://gitlab.com/gitlab-org/gitlab-ee/issues/9713, and looks like they are getting around to it, but it will be first implemented in the paid version only (which is usually what they do first, and later they move some features from paid to free). Here is their list of issues sorted by popularity in the free and paid versions:

https://gitlab.com/gitlab-org/gitlab-ce/issues?sort=popularity
https://gitlab.com/gitlab-org/gitlab-ee/issues?sort=popularity

That's nice about it, that you can see exactly what they are working on, as well as what the popular requests are and the status of your own requests:

https://gitlab.com/gitlab-org/gitlab-ce/issues?scope=all&state=all&author_username=certik

although sometimes they don't have the manpower to implement everything themselves and rather use 3rd party libraries that don't do things exactly as I would like:

https://gitlab.com/gitlab-org/gitlab-ce/issues/37485

[Upabjojr]

Coroutines have been approved for the C++20 standard (February 2019):
https://www.reddit.com/r/cpp/comments/au0c4x/201902_kona_iso_c_committee_trip_report_c20/