Opened 6 years ago
Closed 6 years ago
#17506 closed enhancement (fixed)
Implement broken circuits and NBC sets
Reported by: | tscrim | Owned by: | tscrim |
---|---|---|---|
Priority: | major | Milestone: | sage-6.5 |
Component: | matroid theory | Keywords: | NBC sets |
Cc: | Stefan, yomcat | Merged in: | |
Authors: | Travis Scrimshaw | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | a0abc54 (Commits, GitHub, GitLab) | Commit: | a0abc5401231b45af845889d7b37d46834326650 |
Dependencies: | Stopgaps: |
Description
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.
Change History (15)
comment:1 Changed 6 years ago by
- Branch set to public/matroids/nbc_sets-17506
- Commit set to 4fa33b61c212a7a7a02b4b5ede61412412beef66
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Commit changed from 4fa33b61c212a7a7a02b4b5ede61412412beef66 to a76ac9149029c2467fc0a69e97f30a9ae67027f7
comment:3 Changed 6 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
ok, looks good to me.
comment:4 Changed 6 years ago by
Thanks Frederic!
comment:5 Changed 6 years ago by
- Status changed from positive_review to needs_work
PDF docs don't build
comment:6 Changed 6 years ago by
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)
comment:7 Changed 6 years ago by
- Commit changed from a76ac9149029c2467fc0a69e97f30a9ae67027f7 to 575b68f9a1b6248d89e2918c1d7cb6c9b92841e3
Branch pushed to git repo; I updated commit sha1. New commits:
575b68f | Fixed my stupidity with latex commands.
|
comment:8 Changed 6 years ago by
\union
is not a valid latex command *facepalm*
@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
.
comment:9 Changed 6 years ago by
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.
comment:10 Changed 6 years ago by
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?
comment:11 Changed 6 years ago by
Well, we try to keep it from becoming like the Graph class. Maybe Frederic can give a third opinion to settle the matter?
comment:12 Changed 6 years ago by
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.
comment:13 Changed 6 years ago by
- Commit changed from 575b68f9a1b6248d89e2918c1d7cb6c9b92841e3 to a0abc5401231b45af845889d7b37d46834326650
Branch pushed to git repo; I updated commit sha1. New commits:
a0abc54 | Remove nbc_sets.
|
comment:14 Changed 6 years ago by
- Status changed from needs_work to positive_review
Yea, but it's a point of contention. Anyways, I've removed nbc_sets
.
comment:15 Changed 6 years ago by
- Branch changed from public/matroids/nbc_sets-17506 to a0abc5401231b45af845889d7b37d46834326650
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Add NBC sets for matroids.