Skip to content

Add Oriented Matroids Package #38024

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 79 commits into
base: develop
Choose a base branch
from
Open

Add Oriented Matroids Package #38024

wants to merge 79 commits into from

Conversation

thecaligarmo
Copy link
Contributor

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
Copy link
Contributor Author

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 GitHub Actions
Copy link

github-actions bot commented May 18, 2024 •
edited
Loading

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

@gmou3
Copy link
Contributor

gmou3 commented May 19, 2024

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 added 2 commits May 28, 2024 14:32
gmou3
gmou3 reviewed May 28, 2024
View reviewed changes
gmou3
gmou3 suggested changes May 28, 2024
View reviewed changes
@thecaligarmo
Copy link
Contributor Author

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

@gmou3
Copy link
Contributor

gmou3 commented Jul 4, 2024

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

@thecaligarmo
Copy link
Contributor Author

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
Copy link
Contributor Author

Ok, I think I fixed all the linting/pyright stuff. I also changed the parent/element situation.

Only thing I didn't do was the settings file. Still trying to see what the best way of handling that is.

@thecaligarmo
Copy link
Contributor Author

For the settings file, I just went ahead and removed the extra settings that I have normally. It feels very weird that the settings file is there, but anyway, I just undid my settings changes.

tscrim
tscrim requested changes Dec 1, 2024
View reviewed changes
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. I think this is better (which can be verified by the fact it passes the TestSuite, right?).

I have a few other changes to improve the efficiency and some details. Hopefully this will be the last round. Let me know if you have any questions. The changes I'm suggesting might need to be done in other files/places than what I mentioned.

tscrim
tscrim requested changes Dec 31, 2024
View reviewed changes
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.

It would also be good if you could add tests for most of the error messages that the codecov is reporting aren't tested. This is good for completeness.

src/sage/matroids/oriented_matroids/oriented_matroid.py
True
"""
if isinstance(other, type(self)):
return self.elements() == other.elements()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you end up with two oriented matroids that have equal elements but unequal groundsets? I am asking because of the hashing uses both.

Copy link
Contributor Author

@thecaligarmo thecaligarmo Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you can have two oriented matroids with equal elements but unequal groundsets. They are (combinatorially) isomorphic, hence why I didn't include them in the equality test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, what's the best way to handle this? Should I make it to be "strictly equal" (aka groundsets must be the same)? Because then it might be confusing since in some sense I'm having it mean "isomorphic" but in another sense, I'm not testing for isomorphism.

src/sage/matroids/oriented_matroids/oriented_matroid.py
return self._circuits
raise NotImplementedError("circuits not implemented")

def cocircuits(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a doctest.

src/sage/matroids/oriented_matroids/oriented_matroid.py Outdated
return False
return True

def is_dual_with(self, other):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a doctest.

src/sage/matroids/oriented_matroids/signed_subset_element.py
ground_set = [str(self(e)) for e in self.groundset()]
return "\\left(" + ','.join(ground_set) + "\\right)"

def __copy__(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, that is what I am suggesting.

@thecaligarmo
Copy link
Contributor Author

It would also be good if you could add tests for most of the error messages that the codecov is reporting aren't tested. This is good for completeness.

Ok, I went ahead and added a few more tests to try and get Codecov higher. I think all of the ones that are left are certificate ones, so I think they should be ok. I'll double check in a few days whether there's a few more tests to add. (Or if there's more you'd like me to add)

@thecaligarmo thecaligarmo mentioned this pull request Jan 14, 2025
Open
53 tasks
@thecaligarmo thecaligarmo requested a review from tscrim March 5, 2025 18:26
@thecaligarmo
Copy link
Contributor Author

Looks like one doctest is failing according to the checks. I'll double check this once I can get my local version of sage working again after upgrading ._.

For my own ease, here's the log message:

sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/matroids/oriented_matroids/oriented_matroid.py  # 1 doctest failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmou3 gmou3 gmou3 approved these changes

@tscrim tscrim Awaiting requested review from tscrim

Requested changes must be addressed to merge this pull request.

Assignees

@gmou3 gmou3

Labels
c: matroid theory s: needs review t: enhancement t: feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thecaligarmo @gmou3 @tscrim @fchapoton