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

add interface to nauty's genktreeg #36924

Merged
merged 7 commits into from
Dec 26, 2023

Conversation

dcoudert
Copy link
Contributor

Nauty has a generator of k-trees. We add an interface to this generator.
This is a complement of #36587.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

@dcoudert
Copy link
Contributor Author

Notice that genktreeg is currently not accessible from the homebrew install of nauty, it is missing in the formula https://github.com/Homebrew/homebrew-core/blob/32c869d54088962356f34b8fe2cb64f4e8740fe9/Formula/n/nauty.rb . I don't know how to report that.

So currently we must install nauty from source to access genktreeg.

@dcoudert
Copy link
Contributor Author

Notice that genktreeg is currently not accessible from the homebrew install of nauty, it is missing in the formula https://github.com/Homebrew/homebrew-core/blob/32c869d54088962356f34b8fe2cb64f4e8740fe9/Formula/n/nauty.rb . I don't know how to report that.

So currently we must install nauty from source to access genktreeg.

Apparently also not the case for the pip installable version (see doctests errors).

@mkoeppe
Copy link
Member

mkoeppe commented Dec 19, 2023

You can change build/pkgs/nauty/spkg-configure.m4 to add a test for this generator

@mkoeppe
Copy link
Member

mkoeppe commented Dec 19, 2023

Right now we only test for features of nauty 2.8.0; this new generator was added in 2.8.8.

@dcoudert
Copy link
Contributor Author

Thank you Matthias. Let me know if I should take the opportunity to add other generators, even if we don't have interfaces yet.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 20, 2023

Let me know if I should take the opportunity to add other generators, even if we don't have interfaces yet.

I think it's good enough to just test for this one - as a representative of the newest features in 2.8.8. It may be a good idea to update the comment in the spkg-configure.m4 file though

@dcoudert
Copy link
Contributor Author

I have updated the comment.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 20, 2023

Let me know if I should take the opportunity to add other generators

Now I realize that you may have meant ... to add them in the installation script? +1 on that.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 20, 2023

Also so that the updated installation script takes effect on users' machines:

diff --git a/build/pkgs/nauty/package-version.txt b/build/pkgs/nauty/package-version.txt
index 80803faf1b9..b8635c72de3 100644
--- a/build/pkgs/nauty/package-version.txt
+++ b/build/pkgs/nauty/package-version.txt
@@ -1 +1 @@
-2.8.8
+2.8.8.p0

@mkoeppe
Copy link
Member

mkoeppe commented Dec 20, 2023

Would it make sense to add # needs nauty to the doctests?

This would help with the Conda CI (which fails because it does not have a suitable nauty package; related to the discussion in #35593 (comment))

@dcoudert
Copy link
Contributor Author

We may add # needs nauty, but I don't know how to detect all the doctests that will need this addition.

Also,; we currently have

src/sage/features/nauty.py:        sage: NautyExecutable('converseg').is_present()         # optional - nauty
src/sage/features/nauty.py:        sage: Nauty().is_present()                              # optional - nauty

@dcoudert
Copy link
Contributor Author

I added some tags. Let's see if it is enough for the Conda CI

@tobiasdiez
Copy link
Contributor

I added some tags. Let's see if it is enough for the Conda CI

Looks like this particular program is not included in the conda distribution. Perhaps the fix is as easy as changing https://github.com/conda-forge/nauty-feedstock/blob/f0bb97fb7503af89d94250541f2519cbc154008e/recipe/build.sh#L28 accordingly.

@dcoudert
Copy link
Contributor Author

dcoudert commented Dec 21, 2023

Several programs of nauty are missing in the conda and homebrew distributions (may be in other distributions as well): countneg, genktreeg, ransubg

PROGRAMS="
- addedgeg addptg amtog ancestorg assembleg biplabg catg complg converseg copyg countg cubhamg deledgeg
- delptg dimacs2g directg dreadnaut dretodot dretog edgetransg genbg genbgL geng gengL genposetg genquarticg genrang
- genspecialg gentourng gentreeg hamheuristic labelg linegraphg listg multig nbrhoodg
- newedgeg pickg planarg productg ranlabg shortg showg subdivideg twohamg underlyingg vcolg
- watercluster2 NRswitchg"
+ addedgeg addptg amtog ancestorg assembleg biplabg catg complg converseg copyg
+ countg countneg cubhamg deledgeg delptg dimacs2g directg dreadnaut dretodot
+ dretog edgetransg genbg genbgL geng gengL genposetg genquarticg genrang
+ genspecialg gentourng gentreeg genktreeg hamheuristic labelg linegraphg listg
+ multig nbrhoodg newedgeg pickg planarg productg ranlabg ransubg shortg showg
+ subdivideg twohamg underlyingg vcolg watercluster2 NRswitchg"

What's the right way to ask for modifications ?

@tobiasdiez
Copy link
Contributor

Just make the edits (say in the web editor of github) and open a PR. Alternatively, I'm sure @isuruf or @saraedum are happy to help.

@dcoudert
Copy link
Contributor Author

This now conda-forge/nauty-feedstock#24

@mkoeppe
Copy link
Member

mkoeppe commented Dec 21, 2023

Also,; we currently have

src/sage/features/nauty.py:        sage: NautyExecutable('converseg').is_present()         # optional - nauty
src/sage/features/nauty.py:        sage: Nauty().is_present()                              # optional - nauty

These should be needs, not optional -- because nauty is a standard package.

./sage -fixdoctests --overwrite --no-test src/sage/features/nauty.py src/sage/graphs/graph_generators.py will fix this automatically for you

@mkoeppe
Copy link
Member

mkoeppe commented Dec 22, 2023

The runtime feature test still thinks that the nauty feature is available, which is why the conda test is still failing. You'll need this:

diff --git a/src/sage/features/nauty.py b/src/sage/features/nauty.py
index 0bbe796e003..4a07264f927 100644
--- a/src/sage/features/nauty.py
+++ b/src/sage/features/nauty.py
@@ -62,7 +62,7 @@ class Nauty(JoinFeature):
         """
         JoinFeature.__init__(self, "nauty",
                              [NautyExecutable(name)
-                              for name in ('directg', 'gentourng', 'geng', 'genbg', 'gentreeg', 'converseg')])
+                              for name in ('directg', 'gentourng', 'geng', 'genbg', 'gentreeg', 'genktreeg')])
 
 
 def all_features():

@dcoudert
Copy link
Contributor Author

Indeed. Fully updating nauty is more involved than what I though.

Copy link

Documentation preview for this PR (built with commit 5ad350c; changes) is ready! 🎉

@mkoeppe
Copy link
Member

mkoeppe commented Dec 22, 2023

Looking good; ready for review?

@dcoudert
Copy link
Contributor Author

Yes, I think it's ready for review.

Copy link
Member

@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 pushed a commit to vbraun/sage that referenced this pull request Dec 24, 2023
    
Nauty has a generator of k-trees. We add an interface to this generator.
This is a complement of sagemath#36587.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36924
Reported by: David Coudert
Reviewer(s): Matthias Köppe
@vbraun vbraun merged commit 980aa72 into sagemath:develop Dec 26, 2023
41 of 52 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 26, 2023
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.

None yet

4 participants