Skip to content

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 17, 2024

📝 Checklist

Increasing the lower bound for system cmake to 3.18. This will cause system cmake on ubuntu-focal to be rejected, so cmake will be built from source on this platform. https://repology.org/project/cmake/versions

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Apr 17, 2024

Documentation preview for this PR (built with commit 4f748fe; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

I have igraph 0.10.11 installed via homebrew

MAC-04017247:sage dcoudert$ ls -l /opt/homebrew/Cellar/igraph/
total 0
drwxr-xr-x  12 dcoudert  admin  384 17 avr 09:15 0.10.11

in a fresh shell session, I did:

make distclean
./bootstrap
source .homebrew-build-env
./configure
make build
./sage -i python_igraph

For some reason, it installs igraph from source and don't use my homebrew version.
How to fix that ?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 17, 2024

Thanks for testing! I had forgotten to check/update the spkg-configure file. Fixed now

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

Installation and tests ok on macOS 12.7.4.

LGTM.

@dcoudert
Copy link
Contributor

I have some doubts about all the failing CI. That's why I'm not seeing this PR to positive review yet.

@szhorvat
Copy link
Contributor

szhorvat commented May 7, 2024

igraph 0.10.12 and python-igraph 0.11.5 are released now. I suggest going directly for these new versions.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

Works fine on macOS with homebrew igraph.

LGTM.

@dcoudert
Copy link
Contributor

dcoudert commented Jun 5, 2024

Some errors are reported by the CI on the uninstall part.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 5, 2024

Not sure what you mean by the "uninstall part" but there is a problem with cmake.
test / linux (fedora-30, standard)

#29 40.36   [igraph-0.10.12]   No spkg-legacy-uninstall script; nothing to do
#29 40.36   [igraph-0.10.12]   [spkg-install] Configuring igraph-0.10.12 with cmake
#29 40.36   [igraph-0.10.12]   [spkg-install] CMake Error at CMakeLists.txt:8 (cmake_minimum_required):
#29 40.36   [igraph-0.10.12]   [spkg-install]   CMake 3.18 or higher is required.  You are running version 3.17.2
#29 40.36   [igraph-0.10.12]   [spkg-install] 
#29 40.36   [igraph-0.10.12]   [spkg-install] 
#29 40.36   [igraph-0.10.12]   [spkg-install] -- Configuring incomplete, errors occurred!
#29 40.36   [igraph-0.10.12]   [spkg-install] ********************************************************************************
#29 40.36   [igraph-0.10.12]   [spkg-install] Error configuring igraph-0.10.12 with cmake
#29 40.36   [igraph-0.10.12]   [spkg-install] ********************************************************************************

@dcoudert
Copy link
Contributor

dcoudert commented Jun 5, 2024

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 5, 2024

What I see there is this test failure:

sage -t --random-seed=302171850113617614625936987131470123946 src/sage/graphs/generators/random.py
#30 2631.5 **********************************************************************
#30 2631.5 File "src/sage/graphs/generators/random.py", line 1605, in sage.graphs.generators.random.RandomPartialKTree
#30 2631.5 Failed example:
#30 2631.5     G.treewidth()
#30 2631.5 Expected:
#30 2631.5     5
#30 2631.5 Got:
#30 2631.5     6
#30 2633.3

Do you know something about this test?

@dcoudert
Copy link
Contributor

dcoudert commented Jun 6, 2024

it seems to be

What I see there is this test failure:

sage -t --random-seed=302171850113617614625936987131470123946 src/sage/graphs/generators/random.py
#30 2631.5 **********************************************************************
#30 2631.5 File "src/sage/graphs/generators/random.py", line 1605, in sage.graphs.generators.random.RandomPartialKTree
#30 2631.5 Failed example:
#30 2631.5     G.treewidth()
#30 2631.5 Expected:
#30 2631.5     5
#30 2631.5 Got:
#30 2631.5     6
#30 2633.3

Do you know something about this test?

This seems to be an error from tdlib.

sage: from sage.graphs.graph_decompositions.tree_decomposition import width_of_tree_decomposition
sage: G = Graph('q~~|~q{mLhUo}?v?EO`{?h`?wo@w?fo?As_?lG?Cp_?Uc??{?_?|???Y_G?@gH??{?_?A{???Dp???Dk???Ax????l????Dh????Qo???GkG???_lG????Q_???I@p?G???uO????As????@@gH????FG??A??AW_??A??d????CG@w?C????PE?@????D@G?C???t?O??????')
sage: G.treewidth(algorithm='sage')
5
sage: G.treewidth(algorithm='tdlib')
6
sage: T = G.treewidth(algorithm='tdlib', certificate=True)
sage: width_of_tree_decomposition(G, T)
6

May be we should first try to update tdlib to see if it fixes this issue, and otherwise report upstream.

@dcoudert
Copy link
Contributor

dcoudert commented Jun 6, 2024

I have opened #38159

@dcoudert
Copy link
Contributor

dcoudert commented Jun 6, 2024

This PR looks good to me. Let me know if ready for review (currently in needs work).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2024

Thanks. It's ready for review

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2024

Thank you!

@vbraun vbraun merged commit a09048c into sagemath:develop Jun 9, 2024
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.

4 participants