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:

Status badges

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 tscrim

  • Branch set to public/matroids/nbc_sets-17506
  • Commit set to 4fa33b61c212a7a7a02b4b5ede61412412beef66
  • Status changed from new to needs_review

New commits:

4fa33b6Add NBC sets for matroids.

comment:2 Changed 6 years ago by git

  • Commit changed from 4fa33b61c212a7a7a02b4b5ede61412412beef66 to a76ac9149029c2467fc0a69e97f30a9ae67027f7

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

comment:3 Changed 6 years ago by chapoton

  • 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 tscrim

Thanks Frederic!

comment:5 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

PDF docs don't build

comment:6 Changed 6 years ago by Stefan

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 git

  • Commit changed from a76ac9149029c2467fc0a69e97f30a9ae67027f7 to 575b68f9a1b6248d89e2918c1d7cb6c9b92841e3

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

575b68fFixed my stupidity with latex commands.

comment:8 Changed 6 years ago by tscrim

\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 Stefan

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 tscrim

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 Stefan

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 chapoton

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 git

  • Commit changed from 575b68f9a1b6248d89e2918c1d7cb6c9b92841e3 to a0abc5401231b45af845889d7b37d46834326650

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

a0abc54Remove nbc_sets.

comment:14 Changed 6 years ago by tscrim

  • 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 vbraun

  • Branch changed from public/matroids/nbc_sets-17506 to a0abc5401231b45af845889d7b37d46834326650
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.