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

Upgrade cddlib to 0.94j #25344

Closed
saraedum opened this issue May 11, 2018 · 55 comments
Closed

Upgrade cddlib to 0.94j #25344

saraedum opened this issue May 11, 2018 · 55 comments

Comments

@saraedum
Copy link
Member

and use cddexec instead of cdd_both_reps. cddexec should be faster and is, according to upstream, "done the right way". The change to cddexec and the version upgrade make some numerical issues visible, so some things that worked before are not supported anymore. Additionally, we now require an inexact redundancy-free representation to be convertible back and forth between V and H representation (without loss of vertices/inequalities) before we trust it. Some standard examples, fail this check. I can not really make the call on whether these are bugs in cddlib or numerical issues that could not possibly be fixed (as I do not understand this well enough.)

So why this change? Mostly because we do not patch cddlib anymore and just use it as is. It is also faster in some cases (that's what upstream cared most about when the cddlib author heard about our cdd_both_reps,) e.g.,

%timeit polytopes.six_hundred_cell(exact=False)

went from 993ms to 246ms

Some other examples see minor slowdowns because of the additional transformation that we compute initially to make sure that the conversion back and forth worked out.

Note that we also don't need the patched random number generator as upstream now uses a portable rng.

Tarball: https://github.com/cddlib/cddlib/releases/download/0.94j/cddlib-0.94j.tar.gz

CC: @kiwifb @vbraun @timokau

Component: packages: standard

Author: Julian Rüth

Branch/Commit: u/saraedum/25344 @ 9b7ee99

Reviewer: Erik Bray

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

@embray
Copy link
Contributor

embray commented May 14, 2018

comment:2

Why wait? If we have the tarball we can do it now, and it will be uploaded to Sage's mirrors.

@saraedum
Copy link
Member Author

comment:3

Because I would still like to get your cygwin fixes into upstream.

@embray
Copy link
Contributor

embray commented May 16, 2018

comment:4

Ah, ok.

@saraedum
Copy link
Member Author

Branch: u/saraedum/25344

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

Commit: 3df595d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3df595dUpgrade cddlib

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

Changed commit from 3df595d to 4e87711

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4e87711Upgrade cddlib

@saraedum
Copy link
Member Author

Work Issues: do all doctests pass?

@saraedum
Copy link
Member Author

comment:8

I am running --long doctests for all of Sage currently; everything in geometry/ passes.

@saraedum

This comment has been minimized.

@saraedum

This comment has been minimized.

@saraedum
Copy link
Member Author

Changed work issues from do all doctests pass? to do all doctests pass?, the tarball is not final yet

@saraedum
Copy link
Member Author

Changed work issues from do all doctests pass?, the tarball is not final yet to the tarball is not final yet

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2018

Changed commit from 4e87711 to af9d5be

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2018

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

14f2c32Fix failing doctests
af9d5beRelax cddlib sanity checks

@saraedum

This comment has been minimized.

@saraedum
Copy link
Member Author

comment:15

ccing people who previously touched the cddlib integration code.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b9a9a03Upgrade cddlib
9a649fcFix failing doctests
c404867Relax cddlib sanity checks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2018

Changed commit from af9d5be to c404867

@saraedum
Copy link
Member Author

Changed work issues from the tarball is not final yet to do all doctests pass?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2018

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

32987a6Update hashes with actual release hashes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2018

Changed commit from c404867 to 32987a6

@saraedum
Copy link
Member Author

New commits:

32987a6Update hashes with actual release hashes

@saraedum saraedum changed the title Upgrade cddlib to 0.94j (once it's released) Upgrade cddlib to 0.94j Jun 21, 2018
@embray
Copy link
Contributor

embray commented Jun 22, 2018

comment:20

I assume this has the Cygwin fixes in it now?

@timokau
Copy link
Contributor

timokau commented Jul 11, 2018

comment:27

https://www.inf.ethz.ch/personal/fukudak/cdd_home/index.html still points to 0.94i.

@saraedum
Copy link
Member Author

comment:28

It has the author's approval. However, the author is considering to abandon it if the recent acquisiton of github turns the platform the wrong way.

@kiwifb
Copy link
Member

kiwifb commented Jul 11, 2018

comment:29

In any case I think we would prefer to have a link to an official tarball rather than an attachment - especially for distros (well me in Gentoo in particular).

@timokau
Copy link
Contributor

timokau commented Jul 11, 2018

comment:30

Thanks for your work, that greatly simplifies packaging :) Every patch replaced by something upstream supported is a win.

Could you maybe ask them to make a note on the cddlib homepage? That would help people that stumble there and also make it seem a bit more legit.

@saraedum
Copy link
Member Author

comment:31

Sorry, I wasn't aware of the details of the SPKG upgrade process. Actually, the checksums are for the tarball on github.

@saraedum

This comment has been minimized.

@saraedum
Copy link
Member Author

The latest cddlib release

@saraedum
Copy link
Member Author

comment:32

Attachment: cddlib-0.94j.tar.gz

Replying to @timokau:

Thanks for your work, that greatly simplifies packaging :) Every patch replaced by something upstream supported is a win.

Could you maybe ask them to make a note on the cddlib homepage? That would help people that stumble there and also make it seem a bit more legit.

This is now cddlib/cddlib#18.

@kiwifb
Copy link
Member

kiwifb commented Jul 11, 2018

comment:33

A link is enough. If we are going to use the tarball https://github.com/cddlib/cddlib/releases/download/0.94j/cddlib-0.94j.tar.gz then spkg-src is not needed. As it been prepared with make sdist (and then why not sdist-bzip2 or whatever the option is)? If so there shouldn't be any intermediate autotools files to clean.

@saraedum
Copy link
Member Author

comment:34

Replying to @kiwifb:

A link is enough. If we are going to use the tarball https://github.com/cddlib/cddlib/releases/download/0.94j/cddlib-0.94j.tar.gz then spkg-src is not needed. As it been prepared with make sdist (and then why not sdist-bzip2 or whatever the option is)? If so there shouldn't be any intermediate autotools files to clean.

The tarball came out of make distcheck. I don't understand the question about the intermediate autotools files.

@kiwifb
Copy link
Member

kiwifb commented Jul 12, 2018

comment:35

Replying to @saraedum:

Replying to @kiwifb:

A link is enough. If we are going to use the tarball https://github.com/cddlib/cddlib/releases/download/0.94j/cddlib-0.94j.tar.gz then spkg-src is not needed. As it been prepared with make sdist (and then why not sdist-bzip2 or whatever the option is)? If so there shouldn't be any intermediate autotools files to clean.

The tarball came out of make distcheck. I don't understand the question about the intermediate autotools files.

Good enough I guess, although I never used that target. The question about intermediate autotools files is related to spkg-src, I looked at what it is supposed to do. spkg-src is usually to take upstream and then produce a new tarball for distribution in sage for whatever reasons. That particular spkg-src mostly does cleaning. But if we use the tarball produced by make distcheck we don't need it at all and can remove it.

@videlec
Copy link
Contributor

videlec commented Aug 3, 2018

comment:36

update milestone 8.3 -> 8.4

@videlec videlec modified the milestones: sage-8.3, sage-8.4 Aug 3, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2018

Changed commit from c4b0f10 to 9b7ee99

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2018

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

9b7ee99Merge remote-tracking branch 'trac/develop' into 25344

@saraedum
Copy link
Member Author

saraedum commented Aug 5, 2018

comment:38

trivial merge

@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

comment:39

Don't modify positively reviewed tickets unless you have a reason. There was no merge conflict, so no need to do anything. I'll ignore the superfluous merge.

@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

New commits:

9b7ee99Merge remote-tracking branch 'trac/develop' into 25344

@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

New commits:

9b7ee99Merge remote-tracking branch 'trac/develop' into 25344

@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

comment:43

The trac plugin automatically updates the commit field to the branch head, even though this is not the version that was merged (=c4b0f10c614c7bfa5efaa8ab3908ccb687e992e4).

@saraedum
Copy link
Member Author

saraedum commented Aug 5, 2018

comment:44

I needed a clean patch against 8.3 for Debian and Conda distribution so I decided to push the merge so that I can say in the packaging scripts that this is actually the patch exactly as it is going into upstream.

I am sorry if this caused some trouble for you. I didn't see the harm of merging in develop. What is the problem other than some noise here on the ticket?

@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

comment:45

The problem is that I find that the branch has changed after running the buildbot, when I'm about to close the ticket....

@saraedum
Copy link
Member Author

saraedum commented Aug 5, 2018

comment:46

I see. Sorry for that then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants