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

Implement broken circuits and NBC sets #17506

Closed
tscrim opened this issue Dec 15, 2014 · 21 comments
Closed

Implement broken circuits and NBC sets #17506

tscrim opened this issue Dec 15, 2014 · 21 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Dec 15, 2014

Broken circuits and no broken circuit (NBC) sets are important aspects of matroid theory. In particular, a basis for the Orlik-Solomon algebra can be given by NBC sets. This implements a basic iterator for broken circuits and NBC sets.

CC: @sagetrac-Stefan @sagetrac-yomcat

Component: matroid theory

Keywords: NBC sets

Author: Travis Scrimshaw

Branch/Commit: a0abc54

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/17506

@tscrim tscrim added this to the sage-6.5 milestone Dec 15, 2014
@tscrim tscrim self-assigned this Dec 15, 2014
@tscrim
Copy link
Collaborator Author

tscrim commented Dec 15, 2014

Commit: 4fa33b6

@tscrim
Copy link
Collaborator Author

tscrim commented Dec 15, 2014

Branch: public/matroids/nbc_sets-17506

@tscrim
Copy link
Collaborator Author

tscrim commented Dec 15, 2014

New commits:

4fa33b6Add NBC sets for matroids.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 21, 2014

Changed commit from 4fa33b6 to a76ac91

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 21, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

0eb8837Merge branch 'public/matroids/nbc_sets-17506' of trac in 6.5.b3
a76ac91trac #17506 pep8 detail

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:3

ok, looks good to me.

@tscrim
Copy link
Collaborator Author

tscrim commented Dec 21, 2014

comment:4

Thanks Frederic!

@vbraun
Copy link
Member

vbraun commented Dec 21, 2014

comment:5

PDF docs don't build

@sagetrac-Stefan
Copy link
Mannequin

sagetrac-Stefan mannequin commented Dec 21, 2014

comment:6

I'm sorry, I didn't look at this ticket before. You add TWO methods to Matroid that do the same thing:

Matroid.no_broken_circuit_sets

and

Matroid.nbc_sets.

This is needlessly cluttering up the list of methods a user sees through autocomplete. Please keep one of the two (preferably the long one, as it is more descriptive)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 21, 2014

Changed commit from a76ac91 to 575b68f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 21, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

575b68fFixed my stupidity with latex commands.

@tscrim
Copy link
Collaborator Author

tscrim commented Dec 21, 2014

comment:8

\union is not a valid latex command facepalm

@sagetrac-Stefan, if we are going to keep only one, I would say lets keep nbc_sets since that is how they are more commonly known. However because of this and it is good to have methods with fully spelled out names, I think we should keep both. If you strongly believe there should only be one, then I will remove no_broken_circuit_sets.

@sagetrac-Stefan
Copy link
Mannequin

sagetrac-Stefan mannequin commented Dec 21, 2014

comment:9

I disagree. People in the field will know what NBC stands for and can find the long name. To others, the long name immediately tells them what the thing does, as opposed to the mysterious abbreviation.

I think clarity should come before brevity.

@tscrim
Copy link
Collaborator Author

tscrim commented Dec 22, 2014

comment:10

I think the short name, being a very common abbreviation, with the documentation is sufficient. Although I still would argue that having both methods gives the option to the user; plus it's not like matroids are current overflowing with methods (as opposed to Graphs with nearly 300). Shall we see what Federic thinks and just go with that?

@sagetrac-Stefan
Copy link
Mannequin

sagetrac-Stefan mannequin commented Dec 23, 2014

comment:11

Well, we try to keep it from becoming like the Graph class. Maybe Frederic can give a third opinion to settle the matter?

@fchapoton
Copy link
Contributor

comment:12

I would vote for keeping only the long name. The "very common abreviation" NBC is not so universally well-known. But this seems to me to be a very minor point.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 24, 2014

Changed commit from 575b68f to a0abc54

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 24, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

a0abc54Remove nbc_sets.

@tscrim
Copy link
Collaborator Author

tscrim commented Dec 24, 2014

comment:14

Yea, but it's a point of contention. Anyways, I've removed nbc_sets.

@vbraun
Copy link
Member

vbraun commented Jan 2, 2015

Changed branch from public/matroids/nbc_sets-17506 to a0abc54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants