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 Brettell's matroids and reorganize database #36842

Merged
merged 18 commits into from
Jan 22, 2024
Merged

Conversation

gmou3
Copy link
Contributor

@gmou3 gmou3 commented Dec 8, 2023

This PR reorganizes the catalog of available matroids into separate files (and inside a new "database" folder). It also expands the catalog with the addition of Brettell's "interesting matroids" (as found here: https://homepages.ecs.vuw.ac.nz/~bretteni/code.html). It accurately reorders and fills some gaps in the matroids found in Oxley's Matroid Theory appendix.

It also adds some functions to easily access these collection of matroids. This can be useful to easily experiment and check conjectures.

This PR partially addresses Issue #36014.

Some changes require an update of the documentation.

📝 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

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 8, 2023

@xuluze

@gmou3 gmou3 requested a review from mkoeppe December 11, 2023 20:45
@gmou3 gmou3 force-pushed the database branch 3 times, most recently from 3c9b67d to f2795b5 Compare December 16, 2023 02:21
@gmou3
Copy link
Contributor Author

gmou3 commented Dec 16, 2023

I have updated the documentation and got rid of some build errors. (There still appears an error when testing my fork; an html file not available at the "Copy docs" stage of the doc build. Not sure how to fix this.)

Can we maybe get a rerun of the tests here? @mkoeppe

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Overall, this seems fine with me. Although your way of listing the matroids is very strange from a programming perspective. I would just have a list, but organize the list into the appropriate blocks.

Also, how do you want to deal with the redundancy between the catalog and the named_matroids? It seems like you are making things more complicated as stuff could fall out of sync easily.

src/sage/matroids/database/driver_functions.py Outdated Show resolved Hide resolved
src/sage/matroids/database/driver_functions.py Outdated Show resolved Hide resolved
src/sage/matroids/database/driver_functions.py Outdated Show resolved Hide resolved
@gmou3
Copy link
Contributor Author

gmou3 commented Dec 20, 2023

Overall, this seems fine with me. Although your way of listing the matroids is very strange from a programming perspective. I would just have a list, but organize the list into the appropriate blocks.

I could move all code under the folder database into one database.py file. This would be more minimal in terms of code files and documentation pages. However it would be a bit less structured for the programmer. Overall I think that it is indeed preferable in one file. I am planning to do more PRs (adding AllMatroids function, relabeling matroids, etc.) and I will probably do this (but I can do it here if you want).

Also, how do you want to deal with the redundancy between the catalog and the named_matroids? It seems like you are making things more complicated as stuff could fall out of sync easily.

Glad you noticed this as I was planning to bring it up. I would remove the named_matroids module and only keep the catalog module. I find it better as a name (and shorter). The reason I kept both is as to not break any external script that would work with previous versions of sage.

All other minor suggested changes have been implemented, and, seizing the chance, I added a few more parametrized Oxley matroids, the code for which I wrote.

@gmou3 gmou3 requested a review from tscrim December 20, 2023 10:17
@tscrim
Copy link
Collaborator

tscrim commented Dec 21, 2023

Overall, this seems fine with me. Although your way of listing the matroids is very strange from a programming perspective. I would just have a list, but organize the list into the appropriate blocks.

I could move all code under the folder database into one database.py file. This would be more minimal in terms of code files and documentation pages. However it would be a bit less structured for the programmer.

Sorry, I realize my comment was vague. I was talking about the, say, OxleyMatroids function. Constructing a dict and then converting that to a list is overly complicated.

Overall I think that it is indeed preferable in one file. I am planning to do more PRs (adding AllMatroids function, relabeling matroids, etc.) and I will probably do this (but I can do it here if you want).

Indeed, your functions are redoing the work that you have done by splitting them into separate files. It might make sense to instead simply have the functions iterate over the modules following, e.g., https://stackoverflow.com/questions/21885814/how-to-iterate-through-a-modules-functions. That being said, like you, I also lean towards having them all in one file. However, I leave the decision up to you as you have more of a vision for this than I do.

Also, how do you want to deal with the redundancy between the catalog and the named_matroids? It seems like you are making things more complicated as stuff could fall out of sync easily.

Glad you noticed this as I was planning to bring it up. I would remove the named_matroids module and only keep the catalog module. I find it better as a name (and shorter). The reason I kept both is as to not break any external script that would work with previous versions of sage.

There are two ways to approach this: The first would be simply to have an alias (so they are actually the same list). The second would be to issue a deprecation warning to named_matroids, which would then be removed after a year. I just wouldn't do things manually. My preference would be having an alias, but it is a very slight preference.

Delete database folder and create two new files, database_matroids.py, and database_functions.py. Delete named_matroids.py. Update documentation and replace any occurence of "named_matroids" to "catalog". Create "catalog" alias "named_matroids" so that old scripts still work (deprecate/remove in the future).
@gmou3
Copy link
Contributor Author

gmou3 commented Dec 21, 2023

Sorry, I realize my comment was vague. I was talking about the, say, OxleyMatroids function. Constructing a dict and then converting that to a list is overly complicated.

I quite like the dict because they are kept organized by number of elements. It is a good way to keep them in manageable chunks. Especially now with the matroids being moved into one file (database_matroids.py), it becomes clumsier to have them automatically imported (the file also contains parametrized matroids that I don't want to import).

All other recommendations I implemented and the result is becoming rather satisfactory I think. :)

Non spanning circuits uniquely define a matroid, similar to circuits. They are a concise way to define some matroids, like the newly added R9 and NonDesargues. The addition of these 2 matroids completes the list of nonparametrized matroids in Oxley's appendix (There is still one parametrized matroid missing, Q_r).
@tscrim
Copy link
Collaborator

tscrim commented Dec 28, 2023

Sorry, I realize my comment was vague. I was talking about the, say, OxleyMatroids function. Constructing a dict and then converting that to a list is overly complicated.

I quite like the dict because they are kept organized by number of elements. It is a good way to keep them in manageable chunks. Especially now with the matroids being moved into one file (database_matroids.py), it becomes clumsier to have them automatically imported (the file also contains parametrized matroids that I don't want to import).

I don't understand this. Where the matroids are located has nothing to do with how the function is implemented. I fully agree with you that it is useful to have them organized by rank as output. By having the implementation be a dict that gets converted to a list, you make the function logic more complicated than necessary when you are simply constructing a list. Code comments can do the job just as well IMO:

    lst = [
        U24, # rank 4
        U25, U35, # rank 5
        K4, Whirl3, Q6, P6, U36, R6, # rank 6
        Fano, FanoDual, NonFano, NonFanoDual, O7, P7, # rank 7
        # rank 8
            AG32, AG32prime,
            R8, F8, Q8, L8, S8,
            Vamos, T8, J, P8, P8pp,
            Wheel4, Whirl4,
        K33dual, K33, AG23, TernaryDowling3, Pappus, NonPappus, # rank 9
        K5, K5dual, R10, # rank 10
        R12, ExtendedTernaryGolayCode, T12, # rank 12
        PG23 # rank 13
    ]

Although if you added some additional optional arguments (such as as_dict or rank), then it would make sense to organize it first into a dict of lists.

@tscrim
Copy link
Collaborator

tscrim commented Dec 29, 2023

Thank you. This is looking good now. One small change that you can decide. Since you already create the list, I would simply return it (plus it is not going to grow based on user input). The other option would be to use the Python3 yield from lst.

@gmou3
Copy link
Contributor Author

gmou3 commented Dec 29, 2023

The list contains the references to the functions, which I only call when yielding. So it makes sense I think.

@tscrim
Copy link
Collaborator

tscrim commented Dec 29, 2023

Ah, right, you do something with the elements. Then it's no problem. I will do one more look-over in the next day or two, but any changes will likely be small.

@tscrim
Copy link
Collaborator

tscrim commented Jan 1, 2024

As a technical point, we shouldn't delete files but instead leave a redirect from new_file import * that also raises a deprecation.

The other thing is we (generally and are moving towards uniformity) end the docstrings as

def foo():
    """
    Some doc

     EXAMPLES:: 

        sage: foo()
        5
    """
    return 5

with no blanklines; this includes after REFERENCES:. Compare with the previous catalog.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you for all the changes. I think this is now ready to be included into Sage.

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 15, 2024

Great, thank you @tscrim.

I did some small refinements and added a few extra tests given the merging of #36962.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
This PR reorganizes the catalog of available matroids into separate
files (and inside a new "database" folder). It also expands the catalog
with the addition of Brettell's "interesting matroids" (as found here:
https://homepages.ecs.vuw.ac.nz/~bretteni/code.html). It accurately
reorders and fills some gaps in the matroids found in Oxley's Matroid
Theory appendix.
<!-- Why is this change required? What problem does it solve? -->
It also adds some functions to easily access these collection of
matroids. This can be useful to easily experiment and check conjectures.
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
This PR partially addresses Issue sagemath#36014.
<!-- If your change requires a documentation PR, please link it
appropriately. -->
Some changes require an update of the documentation.

### 📝 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#36842
Reported by: gmou3
Reviewer(s): gmou3, Matthias Köppe, Travis Scrimshaw
@tscrim tscrim self-requested a review January 16, 2024 14:16
@tscrim
Copy link
Collaborator

tscrim commented Jan 16, 2024

Great, thank you @tscrim.

I did some small refinements and added a few extra tests given the merging of #36962.

Please try to avoid doing that after a positive review as I now have to review your new changes. It is generally easier and better to just do a new PR when making further improvements (as opposed to fixes).

You also made a more substantive change with the name of the Fano dual matriod. While this is minor as the matroid was not in the catalog before, it would be appreciated if this had been mentioned.

I will try to check in the next day or two, but you could help me by running sage -tp --warn-long [files_changed] and seeing if any tests take longer than half a second. If they do, please mark them as # long time. (I am mainly looking at the change on line 751 for F8 in database_matoids.py.)

Since changes are being made, looking more closely at the files database_matroids.py and database_functions.py, the "titles" of the modules is somewhat strange in comparison to the file names. Should database_matroids.py be titled "Database of matroids"? On the other hand, I feel like database_functions.py is better named as database_collections.py too.

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 16, 2024

Please try to avoid doing that after a positive review as I now have to review your new changes. It is generally easier and better to just do a new PR when making further improvements (as opposed to fixes).

Excuse me for that; noted for next time.

Since changes are being made, looking more closely at the files database_matroids.py and database_functions.py, the "titles" of the modules is somewhat strange in comparison to the file names. Should database_matroids.py be titled "Database of matroids"? On the other hand, I feel like database_functions.py is better named as database_collections.py too.

These names are supposed to be a "tree" description, meaning that "database" is the word of highest significance, and the next word describes the contents. I think the database_collections.py suggestion is good and I will implement it.

@tscrim
Copy link
Collaborator

tscrim commented Jan 17, 2024

Please try to avoid doing that after a positive review as I now have to review your new changes. It is generally easier and better to just do a new PR when making further improvements (as opposed to fixes).

Excuse me for that; noted for next time.

No problem. For the record, I was making just a neutral comment about it.

Since changes are being made, looking more closely at the files database_matroids.py and database_functions.py, the "titles" of the modules is somewhat strange in comparison to the file names. Should database_matroids.py be titled "Database of matroids"? On the other hand, I feel like database_functions.py is better named as database_collections.py too.

These names are supposed to be a "tree" description, meaning that "database" is the word of highest significance, and the next word describes the contents.

That was clear to me, but I don't quite see the relevance to my comment.

Anyways, I think Matroids in the database is better, but there is no "the" database. I still say that "Database of matroids" is the most clear header for the file (and hence the doc link).

Rename "database_functions.py" to "database_collections.py", retitle "database_matroids.py" from "Documentation of matroids" to "Database of matroids", recheck tests for excessive compute time (no changes required)
Copy link

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

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

I believe this is now ready for inclusion. Thank you for all of the changes.

Feel free to request a review from me for anything matroid related. I am not an expert, but I know my way around the area to some degree (including the code). I am also happy to review code outside my expertise too.

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 19, 2024

Certainly. I will send more PRs after this is merged. Thanks.

@vbraun vbraun merged commit 9a62034 into sagemath:develop Jan 22, 2024
17 of 20 checks passed
@gmou3 gmou3 deleted the database branch January 22, 2024 16:45
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 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