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

Support for tachyon >= 0.99.2 #34995

Merged
merged 2 commits into from
Mar 3, 2023
Merged

Support for tachyon >= 0.99.2 #34995

merged 2 commits into from
Mar 3, 2023

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Feb 6, 2023

Taken from #23712.

This is necessary to support tachyon >= 0.99.2, without losing support for older versions of tachyon.

In case #23712 gets delayed, it'd be nice to include this so nothing breaks if system tachyon is updated. This is the first commit from the branch there which already had positive review (the actual update of tachyon has an issue mentioned in #23712 (comment).)

From the commit log:

In tachyon 0.99.2 the keyword focallength was changed to focaldist. To support it, when running on version >= 0.99.2 we "patch" the model as constructed by class sage.plot.plot3d.tachyon.Tachyon.

In the future (possibly when tachyon in sage gets upgraded), all the focallength occurences in sage.plot.plot3d.tachyon can be replaced by focaldist for consistency with new tachyon, and the logic here can be reversed (i.e. patch the model when self.version() < '0.99.2') or just drop support for old versions.

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023

Codecov Report

Base: 88.59% // Head: 88.58% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (1ccd607) compared to base (104dde9).
Patch coverage: 90.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #34995      +/-   ##
===========================================
- Coverage    88.59%   88.58%   -0.01%     
===========================================
  Files         2136     2136              
  Lines       396142   396152      +10     
===========================================
- Hits        350948   350945       -3     
- Misses       45194    45207      +13     
Impacted Files Coverage Δ
src/sage/interfaces/tachyon.py 87.93% <90.00%> (+0.43%) ⬆️
src/sage/interfaces/qsieve.py 71.30% <0.00%> (-2.61%) ⬇️
src/sage/groups/additive_abelian/qmodnz.py 91.48% <0.00%> (-2.13%) ⬇️
src/sage/rings/polynomial/pbori/parallel.py 59.82% <0.00%> (-1.79%) ⬇️
src/sage/modular/arithgroup/congroup_gamma0.py 94.41% <0.00%> (-0.56%) ⬇️
src/sage/graphs/generic_graph.py 89.12% <0.00%> (-0.42%) ⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py 94.09% <0.00%> (-0.40%) ⬇️
src/sage/modular/overconvergent/hecke_series.py 98.76% <0.00%> (-0.31%) ⬇️
src/sage/crypto/sboxes.py 96.17% <0.00%> (-0.28%) ⬇️
src/sage/quadratic_forms/ternary_qf.py 67.12% <0.00%> (-0.20%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tornaria
Copy link
Contributor Author

tornaria commented Feb 8, 2023

I don't understand what is going on here. This is based on the same commit of develop branch as #34994 (i.e. 104dde9, whiich is the current one). The only difference between the two PR branches is my commits, but that doesn't seem responsible for the failures here...

@collares
Copy link
Contributor

collares commented Feb 8, 2023 via email

@tornaria
Copy link
Contributor Author

tornaria commented Feb 9, 2023

I think Debian installs the "no X" version of tachyon by default (https://packages.debian.org/sid/tachyon-bin-nox). Maybe that has a different output format and the regexp fails to find the version? The line containing the version string seems to be printed by demosrc/main.c, a file which includes X libraries.

Indeed 🤦

In tachyon 0.99.2 the keyword `focallength` was changed to `focaldist`.
To support it, when running on version >= 0.99.2 we "patch" the model as
constructed by class `sage.plot.plot3d.tachyon.Tachyon`.

In the future (possibly when tachyon in sage gets upgraded), all the
focallength occurences in sage.plot.plot3d.tachyon can be replaced by
focaldist for consistency with new tachyon, and the logic here can be
reversed (i.e. patch the model when self.version() < '0.99.2') or just
drop support for old versions.
tornaria added a commit to tornaria/sage that referenced this pull request Feb 9, 2023
Debian patches tachyon so it won't report the version.

We hardcode '0.99' since that's the version they ship.
@tornaria
Copy link
Contributor Author

tornaria commented Feb 9, 2023

The issue was that debian patches tachyon binary, and it won't print its version in the same place as upstream tachyon.

Now when there is no version we default to 0.99, which is what debian has been shipping forever, and since it is less than 0.99.2 it will assume it uses the old api (which is identical behaviour as before this PR, so nothing is lost here).

In summary, this PR will have zero effect for tachyon binaries that either report a version < 0.99.2, or don't report any version. But now a binary of tachyon that reports itself as 0.99.5 (current version) will work.

Thanks @collares for spotting this. I didn't realize tachyon was used so much in the testsuite so I was confused by the number of failures. Let's hope now everything passes.

@tornaria
Copy link
Contributor Author

tornaria commented Feb 9, 2023

This is just bad luck (see #32773)

sage -t --random-seed=271029322510964146686204490270134103594 sage/schemes/toric/sheaf/klyachko.py  # 1 doctest failed

This occasional test failure is annoying, maybe it should be marked as # random util it is fixed, because it is a pity to waste 2-3 hours of doctesting for this...

How do I restart the test?

Debian patches tachyon so it won't report the version.

We hardcode '0.99' since that's the version they ship.
@tornaria
Copy link
Contributor Author

I force-pushed without any changes so the tests run again.

Copy link
Contributor

@collares collares left a comment

Choose a reason for hiding this comment

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

NixOS has been shipping tachyon 0.99.5 for a while, and this patch is cleaner than the one we're using. I've tested it and it works great.

Copy link
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM

@vbraun vbraun merged commit bf6c233 into sagemath:develop Mar 3, 2023
@tornaria tornaria deleted the tachyon branch March 6, 2023 20:16
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.

5 participants