[1.0] Update book examples #4205

lkastner · 2024-10-15T09:41:25Z

Examples from modified book source migrated here.

fingolfin · 2024-10-15T09:47:27Z

test/book/specialized/fang-fourier-monomial-bases/oscar-vs-gap.jlcon

@@ -0,0 +1 @@
+basis_lie_highest_weight(:A, 4, [1,3,2,1]);


There is a "bug" here in the bug (just fixed it there):

Suggested change

basis_lie_highest_weight(:A, 4, [1,3,2,1]);

julia> basis_lie_highest_weight(:A, 4, [1,3,2,1]);

I think this is just a code snippet in the book used to produce timings that is not supposed to have any output associated with it. ping @gfourier

Edit: This file was already in the first round of the book but was not imported here back then.

@benlorenz @lkastner so should this file (and the other two "non-code" jlcon files) be put into a "do not include list" somewhere and dropped here?

This file can and will be run with or without the julia> , with the new error checking it should also fail the tests if it triggers any errors. See here: https://github.com/oscar-system/Oscar.jl/actions/runs/11382378478/job/31665741305?pr=4205#step:9:812

I have added the other two non-julia files to the skipped list: 29c62b8 (#4205)

fingolfin · 2024-10-15T09:47:54Z

test/book/specialized/fang-fourier-monomial-bases/gap.jlcon

+L:= SimpleLieAlgebra("A", 4, Rationals);; 
+V:= HighestWeightModule(L, [1,3,2,1]);; 


Ugh, this is GAP code. Will see how we can resolve this in the book (we don't want that tested as Julia code, it can't work)

same here. this is just a code snipped used to produce timings, so this should neither have output nor get tested. ping @gfourier

Edit: This file was already in the first round of the book but was not imported here back then.

lgoettgens · 2024-10-15T10:28:19Z

A lot of the changes here are just reverting changes in show functions that have been added since the 1.0 release, e.g. introduction of Sub-Pc groups, overhauled printing of Groebner bases etc.
Due to these, I expect CI to fail here. Whats the process to handle these cases?

benlorenz · 2024-10-15T11:09:02Z

A lot of the changes here are just reverting changes in show functions that have been added since the 1.0 release, e.g. introduction of Sub-Pc groups, overhauled printing of Groebner bases etc. Due to these, I expect CI to fail here. Whats the process to handle these cases?

This will be switched and rebased to release-1.0.

lgoettgens · 2024-10-15T13:39:22Z

test/book/specialized/fang-fourier-monomial-bases/sl7-cases.jlcon

+[fundamentals, [0, 1, 0, 1, 0], [0, 1, 0, 0, 1], [0, 0, 1, 0, 1]] => 70
+[fundamentals, [1, 0, 0, 1, 0], [0, 1, 0, 1, 0], [0, 1, 0, 0, 1]] => 10
+[fundamentals, [1, 0, 1, 0, 0], [1, 0, 0, 1, 0], [0, 1, 0, 1, 0], [0, 1, 0, 0, 1]] => 10
+[fundamentals, [1, 0, 0, 1, 0], [0, 1, 0, 1, 0], [0, 1, 0, 0, 1], [0, 0, 1, 0, 1]] => 10


this is no code, but just some output formatted as code. This file was already in the first round of the book but was not imported here back then.

lgoettgens · 2024-10-15T13:50:05Z

#4131 was another change to the booktests that you may wanna backport

codecov · 2024-10-15T14:34:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.47%. Comparing base (1ce4c12) to head (35e143c).

Additional details and impacted files

@@             Coverage Diff              @@
##           release-1.0    #4205   +/-   ##
============================================
  Coverage        82.47%   82.47%           
============================================
  Files              563      563           
  Lines            75732    75732           
============================================
  Hits             62463    62463           
  Misses           13269    13269

fingolfin · 2024-10-18T10:34:53Z

So how do we proceed? What is necessary to turn this from a "draft" PR into a full one and then merge it? (Other than fixing that one failing CI test -- I don't understand that failure right now, it involves changed printing of some Polymake objects?!)

benlorenz · 2024-10-18T10:46:37Z

So how do we proceed? What is necessary to turn this from a "draft" PR into a full one and then merge it? (Other than fixing that one failing CI test -- I don't understand that failure right now, it involves changed printing of some Polymake objects?!)

From my point of view this is ready. The main point of this PR was to test the code that is now in the book and this was successful.
We could create a 1.0.5 containing the updated book tests if necessary?

We should prbably do a bit of rebasing to combine some of the commits here but keep the other backport commits separate (and merge without squashing)?

The failing doctest for julia nightly is due to some changes in julia, we did some fixes for such errors on Oscar master but I don't know whether we can or should backport them.

We could probably also look into integrating some of the changes from the book-code into the booktests on master, but that is probably more difficult as that code has evolved since 1.0.

lgoettgens · 2024-10-18T11:45:40Z

We could probably also look into integrating some of the changes from the book-code into the booktests on master, but that is probably more difficult as that code has evolved since 1.0.

I would welcome a rebase of this (with adaptations) on release-1.1 as well, so that our latest release (which will then be 1.1.2) contains the up to date book tests as well. And this may be an nice step for you in bringing them to master.

Updating the changes to 1.1 or master should be no more than re-running all of the inputs and capturing the outputs. Since we don't change the inputs at all, this shouldn't be tooo hard

(cherry picked from commit ec05d52)

* CI: add workaround for new gcc with julia 1.6 on github runners * CI: try using output variable instead of path for julia bindir (cherry picked from commit 43bf5e0)

(cherry picked from commit 466c745)

fingolfin · 2024-10-18T15:53:04Z

For the 1.0 branch we could simply disable tests against Julia nightly; I see no point in trying to make OSCAR 1.0 compatible with Julia 1.12 and later.

Unless it is trivial I would also not both with the 1.1 branch, but rather make a 1.2 release soon. The book is still many weeks or more likely months out (even if they accept it as submitted, it will take time to get it through the pipeline), so by the time people can it, OSCAR 1.2 will be current anyway.

benlorenz · 2024-10-19T12:20:55Z

I cleaned up the commits on this branch, I think this is good now to be merged, preferably without squashing.

I took the diff from this branch (book-import + whitespace fix) and applied it to 1.1 here (together with some other related changes) and for master here. There were only some very minor conflicts.

fingolfin reviewed Oct 15, 2024

View reviewed changes

lkastner force-pushed the lk/update_book_examples branch from 2157630 to 4103888 Compare October 15, 2024 09:48

lgoettgens added the oscar book PRs necessary for the Oscar book label Oct 15, 2024

lgoettgens changed the base branch from master to release-1.0 October 15, 2024 11:36

lgoettgens changed the base branch from release-1.0 to master October 15, 2024 11:36

benlorenz changed the base branch from master to release-1.0 October 15, 2024 13:19

lkastner force-pushed the lk/update_book_examples branch from 4103888 to 6ee7b58 Compare October 15, 2024 13:24

lgoettgens reviewed Oct 15, 2024

View reviewed changes

benlorenz mentioned this pull request Oct 16, 2024

booktest: add error checking for non-jlcon files also on master #4209

Merged

benlorenz and others added 8 commits October 18, 2024 17:36

booktests: some future-proofing (#4172)

bc1e2ca

(cherry picked from commit ec05d52)

booktest: add error-detection for non-jlcon files

bc93a6b

CI: add workaround for new gcc with julia 1.6 on github runners (#4189)

03eb0e0

* CI: add workaround for new gcc with julia 1.6 on github runners * CI: try using output variable instead of path for julia bindir (cherry picked from commit 43bf5e0)

update GCT.jl version

7445d23

booktests: fix version for pkg add to allow patch releases (#4131)

cfd9629

(cherry picked from commit 466c745)

Reimport examples from OSCAR book

7310bde

booktests: redo some whitespace / omission / seed fixes

571179c

booktest: skip some non-julia files

35e143c

benlorenz added the don't squash! do not squash the commits when merging label Oct 18, 2024

benlorenz force-pushed the lk/update_book_examples branch from 5189142 to 35e143c Compare October 18, 2024 15:41

lgoettgens mentioned this pull request Oct 18, 2024

No type piracy: remove matrix*vector methods for Nemo types #760

Open

benlorenz changed the title ~~Update book examples~~ [1.0] Update book examples Oct 18, 2024

This was referenced Oct 18, 2024

[1.1] booktests: merge updates from book repo #4220

Draft

booktests: merge updates from book repo #4221

Draft

benlorenz marked this pull request as ready for review October 19, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.0] Update book examples #4205

[1.0] Update book examples #4205

lkastner commented Oct 15, 2024

fingolfin Oct 15, 2024

lgoettgens Oct 15, 2024 •

edited

Loading

fingolfin Oct 18, 2024

benlorenz Oct 18, 2024

fingolfin Oct 15, 2024

lgoettgens Oct 15, 2024 •

edited

Loading

lgoettgens commented Oct 15, 2024

benlorenz commented Oct 15, 2024

lgoettgens Oct 15, 2024

lgoettgens commented Oct 15, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

fingolfin commented Oct 18, 2024

benlorenz commented Oct 18, 2024

lgoettgens commented Oct 18, 2024

fingolfin commented Oct 18, 2024

benlorenz commented Oct 19, 2024

	basis_lie_highest_weight(:A, 4, [1,3,2,1]);
	julia> basis_lie_highest_weight(:A, 4, [1,3,2,1]);

		L:= SimpleLieAlgebra("A", 4, Rationals);;
		V:= HighestWeightModule(L, [1,3,2,1]);;

[1.0] Update book examples #4205

Are you sure you want to change the base?

[1.0] Update book examples #4205

Conversation

lkastner commented Oct 15, 2024

fingolfin Oct 15, 2024

Choose a reason for hiding this comment

lgoettgens Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

fingolfin Oct 18, 2024

Choose a reason for hiding this comment

benlorenz Oct 18, 2024

Choose a reason for hiding this comment

fingolfin Oct 15, 2024

Choose a reason for hiding this comment

lgoettgens Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

lgoettgens commented Oct 15, 2024

benlorenz commented Oct 15, 2024

lgoettgens Oct 15, 2024

Choose a reason for hiding this comment

lgoettgens commented Oct 15, 2024

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

fingolfin commented Oct 18, 2024

benlorenz commented Oct 18, 2024

lgoettgens commented Oct 18, 2024

fingolfin commented Oct 18, 2024

benlorenz commented Oct 19, 2024

lgoettgens Oct 15, 2024 •

edited

Loading

lgoettgens Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading