Add Oriented Matroids Package

[thecaligarmo]

This pull request merges the Oriented Matroids package that I developed (see https://github.com/thecaligarmo/oriented_matroids) into Sage so that others may develop directly into it rather than relying solely on myself.

This package should resolve ticket #18703 and in addition, my package should be removed from #31164 as this pull request will put Oriented Matroids directly into Sage rather than requiring a package.

If there are any changes needing to be made, just let me know and we can go from there.

📝 Checklist

• 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

[thecaligarmo]

The Build & Test action seems to be failing, but I can't figure out why. It's stating it's not a git repository, when it clearly is. Not sure how to fix it.

[github-actions]

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

[gmou3]

The Build & Test action seems to be failing, but I can't figure out why. It's stating it's not a git repository, when it clearly is. Not sure how to fix it.

Don't worry about that, it is some error of the tester.

[gmou3]

I would recommend all Error messages to start uncapitalized and end without a dot.

[gmou3]

Do something with this commented code?

[gmou3]

Need to add date or complete/delete file.

[thecaligarmo]

@gmou3 I've gone ahead and fixed the things you wanted.

[gmou3]

Thanks. I had opened a PR a while ago with some formatting recommendations. Please have a look and merge or close it please.

[thecaligarmo]

I hadn't noticed the pull request for some reason. I've added your changes as well and double checked things work nicely. Thanks for that.

[thecaligarmo]

Yeah, I think it would be nice to know why something fails when testing out different OMs, I like your solution. I'm sure you will already but if M.is_valid(failure_type=True) is True, we'll want to decide if the format should be the same or different. As in, if it should be (True, '') or True. There's good reasoning for both, but I'm slightly leaning towards the former because then if you loop over things, the returned objects always have the same format.

Alternatively: it makes sense that is_valid would always just return true/false. I also wouldn't be opposed to having a second method called test_validity or something that does the actual test and always returns (Bool, str) so that if you want to test the validity, you run that. I prefer this method because then it's more clear what each function is doing.

If you want I can go ahead and accept the PR you made and also make the changes I suggested? (The one with both a is_valid and a test_validity)

[gmou3]

Good that you point out this complication.

I thought a bit about it and suggest that is_valid(certificate=True) return simply True, as this is the most user-friendly output. Then, M.is_valid(certificate=True) == True would be the condition to check within a loop.

I also prefer the name certificate as it appears elsewhere in the matroids module and, furthermore, we can return further information than the failure type (e.g. two bases for which the exchange axiom fails).

Go ahead and do the changes you want. Either this last suggestion of mine, or the (True, '') variation. The addition of test_validity seems a bit excessive to me.

[thecaligarmo]

If matroids is doing something, might as well go with that option. So then certificate=True is the one that returns True or False else (by default) we get the warnings as is currently happening? (Or did I understand incorrectly)

[tscrim]

I’ve not followed the discussion, but one quick point is that the return for certificate=X should be consistent for any X. So, e.g., if is_valid(certificate=True) returns False, certificate, then it should return True, None (not True). This is very useful when programming and you do not need to analyze the output. (Think about how you would handle such an if statement.)

[thecaligarmo]

I have pushed some changes to thecaligarmo#4. I removed the automatic validity check from the init method in oriented_matroid.py for efficiency's and consistency's sake with the rest of the matroids module.

I think I might have screwed up the pull request. There was a merge with develop and so I couldn't actually tell what you had changed, so I tried to merge mine with develop, but the two branches were different and so I couldn't figure out what was changed and what wasn't. Can you double check that I didn't screw it up?

** Edit: ** Actually, reading your full message, I'm confident I screwed it up since you mentioned that you removed the is_valid test from oriented_matroid, but it's still showing up in my local version. I'll go ahead and fix this while implementing the changes we mentioned for is_valid.

[thecaligarmo]

Ok, I fixed my error with your pull request (For future, probably better not to merge develop into your pull request as then it shows hundreds of files as changed and makes it very difficult to keep track of what was changed [unless I'm looking at the wrong place? But when I tried to compare it was showing so many files had changed so not sure]).

I've also gone ahead and implemented something for is_valid. What I did was:
is_valid() always returns True or False. If you want additional messages then you can add with_errors which will then always return (True/False, "error"). This should make things consistent enough for most use cases.

[gmou3]

It is quite good now I believe. I would change the argument's name from with_errors to certificate. with_errors gives an impression of the method raising errors rather than returning a certificate to help the user/verifier check where the issue is.

Another point I want to raise is the following: I am planning to also implement this for ordinary matroids and I want is_valid(certificate=True) to return more information than the failure type. For example, one return value could be

(False, ('exchange axiom failed', frozenset({1, 2}), frozenset({3, 4}))).

Notice that the certificate is a tuple which always starts with a string and (potentially) continues with other useful info. Is this a good format? If so, should we make the case of an only string certificate also be a tuple for consistency? That is what seems best to me.

[thecaligarmo]

Sounds good, I've changed it to certificate for now pending additional changes.

I think with certificate, I would probably do something like: (Boolean, String, Dictionary). The first component always tells you if it's valid/not; second component gives you the message and the 3rd component will give you "any additional information" (I guess I don't know what the tuple is supposed to be representing? Is it extra information on where the problem occurred?). The reason I put string, dictionary is the dictionary allows us to add extra info in the future if we want it (how long did the is_valid take? how many iterations is it running through? etc.) So it helps with future-proofing slightly. And with a dictionary, we can either do (bool, (string, dict)) or (bool, string, dict) and it's roughly the same, just need to decide which one and go with it.

At the end of the day though, I think it's just preferences and there is no "correct" solution. So if you'd prefer the second part be a tuple instead, can defs do that.

[tscrim]

Two more cents: it should return (bool, dict). The docstring is there to tell the user how to interpret the output. Keep the code simple. You can add verbosity options if you feel displaying that extra data is useful (my guess/thought is that it is not worth adding the complexity to the code).

[thecaligarmo]

That's a good point. I'm also ok with making it (bool, dict) where the string would have key error, reason or something similar. It does keep things even lighter than my proposal.

[tscrim]

I made it only partially through this. My comments in circuit_oriented_matroid.py also apply to the other concrete subclasses.

One bigger design change I would like is to combine the OrientedMatroid function with the class AbstractOrientedMatroid. This makes the user interface easier (e.g., isinstance(M, OrientedMatroid) works as expected), there is one single point of documentation for general information, and the __classcall_private__ mechanism allows for dispatch to subclasses (see, e.g., Partitions).

I will probably have more comments later when I get some more time to go through the rest.

[tscrim]

This needs a doctest, usually a TestSuite(M).run() with M being this class and constructed how a user would.

[thecaligarmo]

Does it make sense to do a TestSuite for each type or should I just do one on the parent class (the one that was called Abstract that will now just become the class?

[thecaligarmo]

One bigger design change I would like is to combine the OrientedMatroid function with the class AbstractOrientedMatroid. This makes the user interface easier (e.g., isinstance(M, OrientedMatroid) works as expected), there is one single point of documentation for general information, and the __classcall_private__ mechanism allows for dispatch to subclasses (see, e.g., Partitions).

The reason this was not done was so that it aligns with the Matroid class. The Matroid class uses a constructor Matroid (in constructor.py) in order to construct a Matroid. They do also have a Matroid object (in matroid.py), but I (personally) think this is confusing since now you have 2 things with the same name. Therefore, I went with AbstractOrientedMatroid for the object definition so that there is no ambiguity between the function and the class.

If we want to merge the two, then I believe we should do a similar thing with the Matroid class/function so that they behave in a similar way.

[tscrim]

I view aligning with the Matroid class as a bit of a fallacy (and we shouldn’t throw good money after bad). That being said, I do think we should make the Matroid class also have the proposed setup. However, that should be done on an independent PR.

Additionally, Abstract in this context has mathematical implications (although this doesn’t necessarily conflict with anything, it does create some confusion as it could be interpreted as an oriented matroid with no concrete realization, which is wrong for its subclasses).

[thecaligarmo]

I view aligning with the Matroid class as a bit of a fallacy (and we shouldn’t throw good money after bad). That being said, I do think we should make the Matroid class also have the proposed setup. However, that should be done on an independent PR.

I think the way it was done is not necessarily a fallacy. The main problem I see with your proposed method is that when we call OrientedMatroid(data), then try and generate the OrientedMatroid, we'll want to create (for example) CovectorOrientedMatroid. But now this class extends OrientedMatroid, calling the __init__ giving us an infinite loop. (There's hack ways around this, by passing a parameter to skip certain things in the main __init__, but that feels weird). We could not call the parent's __init__, but then we have issues that we can't init global things for OrientedMatroids. So in some sense, not having a constructor makes things a little harder to work with.

Do we have an example of an object that does this successfully?

[tscrim]

It should be dispatching to the correct parent class in the __classcall_private__ method. See Partitions for an example (albeit a bit complicated one) IIRC.

[thecaligarmo]

Ok, I've gone ahead and altered the code to use a class instead of a constructor. We'll make a different PR for doing the same to Matroids (Which I believe should be done as well)