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

fix the output of method acyclic_orientations #38757

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dcoudert
Copy link
Contributor

@dcoudert dcoudert commented Oct 3, 2024

This PR fixes the issue with the output of method acyclic_orientations reported in https://ask.sagemath.org/question/79427/the-acyclic-orientations-function-behaves-unexpectedly/.
Now the method outputs digraphs with the correct orientation of the arcs.

📝 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

Copy link

github-actions bot commented Oct 3, 2024

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

@dimpase
Copy link
Member

dimpase commented Oct 3, 2024

Not checking asksage, I've opened #38758 to consider a bigger change.

@dimpase
Copy link
Member

dimpase commented Oct 3, 2024

can we have a shorter doctest - there is no need to list all these 18 things. Instead, something like

sage: g = Graph([(0,1),(1,2),(2,3),(3,0),(0,2)])
sage: len(set([tuple(d.edges(labels=false)) for d in g.acyclic_orientations()]))
18

@dcoudert
Copy link
Contributor Author

dcoudert commented Oct 3, 2024

can we have a shorter doctest - there is no need to list all these 18 things. Instead, something like

sage: g = Graph([(0,1),(1,2),(2,3),(3,0),(0,2)])
sage: len(set([tuple(d.edges(labels=false)) for d in g.acyclic_orientations()]))
18

(I have edited my answer): yes, we can do that

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

apart from a shorter doctest, could you also put the link to askbot in the commit message, and a short explainer?

@dimpase
Copy link
Member

dimpase commented Oct 3, 2024

other than that, good to go

@dcoudert
Copy link
Contributor Author

dcoudert commented Oct 3, 2024

apart from a shorter doctest, could you also put the link to askbot in the commit message, and a short explainer?

I'm not sure to get what you ask for. I can ask a link to this PR :issue:`38757` or to issue #38758 in the doctest. Do you want something more ?

@dimpase
Copy link
Member

dimpase commented Oct 3, 2024

apart from a shorter doctest, could you also put the link to askbot in the commit message, and a short explainer?

I'm not sure to get what you ask for. I can ask a link to this PR :issue:`38757` or to issue #38758 in the doctest. Do you want something more ?

perhaps also something like "return a properly relabeled digraph, not just the arc labeling to describe the orientation" ?
Needless to say, this, and issue #38758 (no need for rst markup), goes after the 2nd line of the commit message.

dcoudert added a commit to dcoudert/sage that referenced this pull request Oct 3, 2024
@dimpase
Copy link
Member

dimpase commented Oct 3, 2024

regarding the commit message, I meant the attached, to replace the branch here
withcommitmessage.patch.txt

return a properly relabeld digraph, not just the arc labeling to
describe the orientation.
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2024
    
This PR fixes the issue with the output of method `acyclic_orientations`
reported in https://ask.sagemath.org/question/79427/the-acyclic-
orientations-function-behaves-unexpectedly/.
Now the method outputs digraphs with the correct orientation of the
arcs.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38757
Reported by: David Coudert
Reviewer(s): Dima Pasechnik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants