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

Matroids: Choose edges of graph as ground set for cycle_matroid #2887

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

lkastner
Copy link
Member

@lkastner lkastner commented Oct 5, 2023

No description provided.

Copy link
Collaborator

@bschroter bschroter left a comment

Choose a reason for hiding this comment

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

The changes look fine to me. But I want to remark the following:

  1. It is odd that a cycle matroid gets now names from the edges of the graph while the dual matroid, i.e., bond or coycle matroid doesn't. This will confuse the user and should be a small fix.

  2. The code assumes that edges are labeled by two nodes (source and destination) which are of type Int Should be more general edge labels and node labels be considered?

@lkastner lkastner force-pushed the lk/cycle_matroid branch 2 times, most recently from d5599c4 to 608f731 Compare October 6, 2023 13:04
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #2887 (608f731) into master (0c82301) will decrease coverage by 0.16%.
Report is 5 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2887      +/-   ##
==========================================
- Coverage   80.55%   80.39%   -0.16%     
==========================================
  Files         456      456              
  Lines       65233    65238       +5     
==========================================
- Hits        52549    52449     -100     
- Misses      12684    12789     +105     
Files Coverage Δ
...ricVarieties/AlgebraicCycles/special_attributes.jl 100.00% <ø> (ø)
src/Combinatorics/Graphs/functions.jl 93.67% <100.00%> (+0.07%) ⬆️
src/Combinatorics/Matroids/matroids.jl 100.00% <100.00%> (ø)

... and 29 files with indirect coverage changes

@lkastner
Copy link
Member Author

1. It is odd that a cycle matroid gets now names from the edges of the graph while the dual matroid, i.e., bond or coycle matroid doesn't. This will confuse the user and should be a small fix.

I do not understand this remark. In your code you have

bond_matroid(g::Graph) = dual_matroid(cycle_matroid(g))

so if dual_matroid works in the right way, this should just work. Let me try:

julia> cm = cycle_matroid(complete_graph(4))
Matroid of rank 3 on 6 elements

julia> dcm = dual_matroid(cm)
Matroid of rank 3 on 6 elements

julia> matroid_groundset(dcm)
6-element Vector{Edge}:
 Edge(2, 1)
 Edge(3, 1)
 Edge(3, 2)
 Edge(4, 1)
 Edge(4, 2)
 Edge(4, 3)

I am not sure what to fix here.

2. The code assumes that edges are labeled by two nodes (source and destination) which are of type `Int` Should be more general edge labels and node labels be considered?

Which code are you referring to? The line I changed in the Matroid section is just

gs = collect(edges(g))

does not assume anything. If you are talking about the conversion, then you are correct. We modelled the Graphs such that they can be used in the same way as Julia's Graphs.jl package in order to make migrating code easier (usually I would call this "compatible", but @lgoettgens frowns up on this). These use only Ints and right now our graphs also only use Ints. Nevertheless if we decide to change this, this change would be in the Graphs section and the Matroid section should not be affected at all.

@bschroter
Copy link
Collaborator

Regarding 1) Yes, you are right. What was irritating me is the change in docs/src/Combinatorics/matroids.md line 29. The rest should automatically work.

Regarding 2) Currently the ground set will take the edges (pairs of nodes), but one might want to have other names, i.e., a labelling of nodes or edges which should then be taken as ground set instead. It is more an open question the a issue.

@lkastner lkastner merged commit c60bda7 into master Oct 11, 2023
9 of 13 checks passed
@lkastner lkastner deleted the lk/cycle_matroid branch October 11, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants