sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.41k stars 475 forks source link

Usainboltz package: fast Boltzmann random generation of tree-like structures #27526

Open Kerl13 opened 5 years ago

Kerl13 commented 5 years ago

Usain Boltz is a !Python/Cython/C++ library that automates the random generation of tree-like structures. It allows to describe a combinatorial structure using a specification in the style of “Analytic Combinatorics” and, using the Boltzmann sampling framework, it produces a fast, uniform, and approximate size sampler of objects described by this specification.

https://gitlab.com/ParComb/usain-boltz

Depends on #34251

CC: @sagetrac-tmonteil @MatthieuDien @Kerl13 @mantepse

Component: packages: optional

Author: Martin Pépin, Matthieu Dien

Branch/Commit: u/MartinPepin/usainboltz @ d8e42b9

Reviewer: Thierry Monteil

Issue created by migration from https://trac.sagemath.org/ticket/27526

edd8e884-f507-429a-b577-5d554626c0fe commented 2 years ago
comment:35

I would replace boltzmann with Boltzmann in the title but apart from that, it looks good to me.

edd8e884-f507-429a-b577-5d554626c0fe commented 2 years ago

Description changed:

--- 
+++ 
@@ -1,13 +1,5 @@
-The Boltzmann method is a framework that describes how to build a uniform random generator for a family of discrete structures from a grammar describing  that family.
+Usain Boltz is a !Python/Cython/C++ library that automates the random generation of tree-like structures.
+It allows to describe a combinatorial structure using a specification in the style of “Analytic Combinatorics” and, using the Boltzmann sampling framework, it produces a fast, uniform, and approximate size sampler of objects described by this specification.

-Our goal is to design a high-level and user-friendly API with :
-* handling of grammars for monovariate/multivariate and unlabeled/labeled/increasingly labeled structures.
-* use of the state of the art oracles (newton iteration, multiparametric tuning, etc)
-* fast sampling of large structures (size 1e6 and more)
- 
-In this ticket we provide a implementation of the Boltzmann method introduced by Duchon, Flajolet, Louchard and Schaeffer (see [DuFlLoSc04](http://algo.inria.fr/flajolet/Publications/DuFlLoSc04.pdf)) with the following features :
-* labeled and unlabeled classes defined with +, *, Seq and recursive definition
-* an oracle dealing with explicit generating function
-* an oracle based on the work of Darasse (see [his thesis](http://www.ortsa.com/lip6_these/docs/these.pdf)) : less accurate but more generic
-* a fast Cython implementation of a generic generator
+https://gitlab.com/ParComb/usain-boltz
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

b2d1bdeCapitalise `Botlzmann' in usainboltz's description
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from e72b3a7 to b2d1bde

mkoeppe commented 2 years ago
comment:38
+Usainboltz: fast Boltzmann random generation of tree-like structures
+====================================================================

this line should start with lowercase

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

5f09d11Lowercase the package name in usainboltz's SPKG.rst
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from b2d1bde to 5f09d11

edd8e884-f507-429a-b577-5d554626c0fe commented 2 years ago

Reviewer: Thierry Monteil

edd8e884-f507-429a-b577-5d554626c0fe commented 2 years ago
comment:40

Once you are happy with your changes, you can set the "Action" to "needs review".

Kerl13 commented 2 years ago
comment:41

Replying to Thierry Monteil:

Once you are happy with your changes, you can set the "Action" to "needs review".

Oh right, thanks for recalling me the usual workflow, I'm not used to it yet ^^

I'm a bit frustrated that I could not test it. I started a new make build which seems to want to recompile some stuff. I'll try to test again tomorrow when it's done and update the Action field afterward

Kerl13 commented 2 years ago
comment:42

Replying to Thierry Monteil:

It worked well for me, i could successfully execute binarytrees.ipynb.

I still don't manage to sage -i usainboltz (even on a fresh clone after having built sage with make build) it seems that usainboltz should appear in the output of make list but it doesn't. Rebuilding build/make/Makefile doesn't help either.

Kerl13 commented 2 years ago
comment:43

Ok now it works! Sorry for the noise, there had been a problem at make configure that I didn't spot when I built sage

mkoeppe commented 2 years ago
comment:45
+++ b/build/pkgs/usainboltz/dependencies
@@ -0,0 +1,4 @@
+$(PYTHON) cython | $(PYTHON_TOOLCHAIN)

It's better to put cython after the |, to make it an order-only dependency

mkoeppe commented 2 years ago
comment:46

On macOS (with clang)

[usainboltz]       gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -iwithsysroot/System/Library/Frameworks/System.framework/PrivateHeaders -iwithsysroot/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/Headers -Werror=implicit-function-declaration -Isrc/xoshiro -Isrc/usainboltz -I/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/local/var/lib/sage/venv-python3.8/include -I/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/Headers -c src/usainboltz/cpp_simulator.cpp -o build/temp.macosx-10.14-x86_64-cpython-38/src/usainboltz/cpp_simulator.o
[usainboltz]       In file included from src/usainboltz/cpp_simulator.cpp:7:
[usainboltz]       src/usainboltz/cpp_simulator.hpp:45:7: error: union member '_as_product' has a non-trivial copy constructor
[usainboltz]           } _as_product;
[usainboltz]             ^
[usainboltz]       src/usainboltz/cpp_simulator.hpp:44:28: note: because field of type 'std::vector<CRule *>' has a user-provided copy constructor
[usainboltz]             std::vector<CRule *> args;
[usainboltz]                                  ^
[usainboltz]       /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/vector:581:5: note: declared here
[usainboltz]           vector(const vector& __x);
[usainboltz]           ^
[usainboltz]       In file included from src/usainboltz/cpp_simulator.cpp:7:
[usainboltz]       src/usainboltz/cpp_simulator.hpp:50:37: error: a space is required between consecutive right angle brackets (use '> >')
[usainboltz]             std::vector<std::vector<double>> weights;
[usainboltz]                                           ^~
[usainboltz]                                           > >
mkoeppe commented 2 years ago
comment:47

I see that cvxpy is a dependency of this package. You may be interested in #34251, which adds it as an SPKG

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 5f09d11 to b00b065

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

b00b065usainboltz: make cython an order-only dependency
Kerl13 commented 2 years ago
comment:49

It's better to put cython after the |, to make it an order-only dependency

Good point, sorry I missed that

Kerl13 commented 2 years ago
comment:50

Replying to Matthias Köppe:

On macOS (with clang)

[usainboltz]       src/usainboltz/cpp_simulator.hpp:50:37: error: a space is required between consecutive right angle brackets (use '> >')
[usainboltz]             std::vector<std::vector<double>> weights;
[usainboltz]                                           ^~

I was not aware of that issue, thanks for reporting, I'll add clang to usainboltz' CI

mkoeppe commented 2 years ago
comment:51

Probably needs -std=c++11 or something like this

Kerl13 commented 2 years ago
comment:52

Replying to Matthias Köppe:

I see that cvxpy is a dependency of this package. You may be interested in #34251, which adds it as an SPKG

Yes it is indeed interesting. How do I benefit from this? Will sage's pip use the optional cvxpy package if I add it as a dependency? Or do I need to switch to the normal package format for usainboltz`? I'll be happy to do so if it's considered better.

Btw, Thierry told me a few days ago that our (only “real” dependency paganini might be of interest for sage as well. Another good reason to package everything as normal?

Kerl13 commented 2 years ago
comment:53

Replying to Matthias Köppe:

Probably needs -std=c++11 or something like this

I'll check that but I think I prefer to follow the error message's suggestion if it makes the library compatible with more compilers / compiler versions

mkoeppe commented 2 years ago
comment:54

See #32074 for detailed info on compiler supported by Sage & related software

Kerl13 commented 2 years ago
comment:55

Replying to Matthias Köppe:

I fixed the compilation issue with clang (at list on my setup).

Would you mind checking whether sage -pip install git+https://gitlab.com/ParComb/usain-boltz#master works for you before I release a new version? That would be really nice.

A few warnings are to be expected because cython does not produce fully c++11 compatible code (https://github.com/cython/cython/pull/4687). I don't know to what extent this is a problem.

On macOS (with clang)

[usainboltz]       gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -iwithsysroot/System/Library/Frameworks/System.framework/PrivateHeaders -iwithsysroot/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/Headers -Werror=implicit-function-declaration -Isrc/xoshiro -Isrc/usainboltz -I/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/local/var/lib/sage/venv-python3.8/include -I/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/Headers -c src/usainboltz/cpp_simulator.cpp -o build/temp.macosx-10.14-x86_64-cpython-38/src/usainboltz/cpp_simulator.o
[...]

By the way, I just noticed you seemed to use gcc from the log you pasted. Is this a mistake or is gcc an alias for clang in your setup?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from b00b065 to 76e06f0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

8392936Add the optas optional package
2397f14Add the paganini optional package
76e06f0Turn usainboltz into a normal package
Kerl13 commented 2 years ago

Dependencies: #34251

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

d8e42b9Bump usainboltz to 0.2.1
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 76e06f0 to d8e42b9

Kerl13 commented 2 years ago
comment:59

I could finally get my hands on a mac this afternoon so I could test the compilation there and release a new version, sorry for the noise

mkoeppe commented 2 years ago
comment:60

Replying to Martin Pépin:

By the way, I just noticed you seemed to use gcc from the log you pasted. Is this a mistake or is gcc an alias for clang in your setup?

On macOS, the gcc binary provided by Xcode is a compatibility shim around Apple's version of clang.

mkoeppe commented 2 years ago
comment:61

Replying to Martin Pépin:

Replying to Matthias Köppe:

I see that cvxpy is a dependency of this package. You may be interested in #34251, which adds it as an SPKG

Yes it is indeed interesting. How do I benefit from this?

34251 is waiting for reviewers...

Will sage's pip use the optional cvxpy package if I add it as a dependency? Or do I need to switch to the normal package format for usainboltz`? I'll be happy to do so if it's considered better.

When #34251 is merged, we should add cvxpy to build/pkgs/usainboltz/dependencies. This will avoid the conflict (possible races in parallel builds) between installing cvxpy from our package and getting cvxpy.

There's no need to switch usainboltz to normal.

mkoeppe commented 2 years ago
comment:62

Replying to Martin Pépin:

Btw, Thierry told me a few days ago that our (only “real” dependency paganini might be of interest for sage as well.

I can't comment on this; it's outside of my domain of mathematical expertise.

Another good reason to package everything as normal?

No, I'd say it's best to set things up as pip packages as you did. The additional maintenance burden (upgrade tickets) with normal packages is typically only warranted if we have tight integration of the provided functionality in Sage.

mkoeppe commented 2 years ago
comment:63

Oh, now I see you have made the switch to normal packages already on the new branch. Then it's best to merge the branch of #34251 here. The current branch can't be tested because cvxpy is not a package

mkoeppe commented 2 years ago
comment:64
diff --git a/build/pkgs/optas/dependencies b/build/pkgs/optas/dependencies
new file mode 100644
index 00000000..7d1ace1
--- /dev/null
+++ b/build/pkgs/optas/dependencies
@@ -0,0 +1,4 @@
+$(PYTHON) | $(PYTHON_TOOLCHAIN) gcc

There's no need for the gcc here. This dependency is implicit for all packages.

mkoeppe commented 2 years ago
comment:65

Other than that, it's looking good

Kerl13 commented 2 years ago
comment:66

Replying to Matthias Köppe:

34251 is waiting for reviewers...

I think I can do that!