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

FiniteStateMachine.__and__ calls intersection and FiniteStateMachine.__or__ calls union. #16016

Closed
sagetrac-skropf mannequin opened this issue Mar 26, 2014 · 15 comments
Closed

Comments

@sagetrac-skropf
Copy link
Mannequin

sagetrac-skropf mannequin commented Mar 26, 2014

Nevertheless, intersection and union are still not implemented. Thus, this only changes the names of the functions. Previously __mul__ called intersection and __add__ called union.

This is an answer to one of the comments in #15078 comment:32.

CC: @dkrenn @cheuberg @seblabbe

Component: combinatorics

Author: Sara Kropf

Branch/Commit: 3c34436

Reviewer: Nathann Cohen

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

@sagetrac-skropf sagetrac-skropf mannequin added this to the sage-6.2 milestone Mar 26, 2014
@sagetrac-skropf
Copy link
Mannequin Author

sagetrac-skropf mannequin commented Mar 26, 2014

Author: Sara Kropf

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 26, 2014

comment:2

I believe that | and + should be aliases for union. That's how it works for sets already

sage: Set([1,2,3])+Set([3,4,6])
{1, 2, 3, 4, 6}
sage: Set([1,2,3])|Set([3,4,6])
{1, 2, 3, 4, 6}

And for graphs only + is defined

sage: graphs.PetersenGraph() + graphs.ChvatalGraph()
Petersen graph disjoint_union Chvatal graph: Graph on 22 vertices
sage: graphs.PetersenGraph() | graphs.ChvatalGraph()
...
TypeError: unsupported operand type(s) for |: 'Graph' and 'Graph'

If you agree with that you can add a __or__ = __add__ after the function's definition :-)

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 27, 2014

comment:4

Well, could you answer yesterday's question ? What do you think ?

Nathann

@sagetrac-skropf
Copy link
Mannequin Author

sagetrac-skropf mannequin commented Mar 27, 2014

comment:5

For me, that sounds good. But I would like to wait for possible other opinions on this topic before I change it, since originally it was suggested to use only __or__ (instead of __add__).

I will wait until Monday.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2014

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

3c34436FiniteStateMachine.__add__ is the same as __or__

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2014

Changed commit from ee5c295 to 3c34436

@sagetrac-skropf
Copy link
Mannequin Author

sagetrac-skropf mannequin commented Mar 31, 2014

comment:7

Now __add__ does the same as __or__, as suggested above.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 31, 2014

comment:8

Oooooooooookayyyyyyyyyyyyyyyyyyyyyyyy !!!

http://grooveshark.com/s/Let+s+Bang/3S4pl4?src=5

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 31, 2014

comment:9

OOops, nononono. It does not pass tests :-)

sage -t --long finite_state_machine.py  # 2 doctests failed

@seblabbe
Copy link
Contributor

comment:10

Replying to @sagetrac-skropf:

For me, that sounds good. But I would like to wait for possible other opinions on this topic before I change it, since originally it was suggested to use only __or__ (instead of __add__).

I will wait until Monday.

I am Ok for __add__ having the same behavior as __or__.

But, do we agree that __mul__ should not link to __and__ ? To me, if A and B are two automata, A * B means the concatenation of them, not the intersection.

Sébastien

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 31, 2014

comment:11

Yepyep, in the current patch we only have an alias from add to or.

Nathann

@sagetrac-skropf
Copy link
Mannequin Author

sagetrac-skropf mannequin commented Mar 31, 2014

comment:12

Replying to @nathanncohen:

OOops, nononono. It does not pass tests :-)

sage -t --long finite_state_machine.py  # 2 doctests failed

I don't know what you mean. For me, all doctests pass.

Can you check it again, please, and tell me your error messages?

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 31, 2014

comment:13

My mistake ! I must have forgotten to recompile or something. Sorry for that, good to go :-)

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 31, 2014

Reviewer: Nathann Cohen

@vbraun
Copy link
Member

vbraun commented Mar 31, 2014

Changed branch from u/skropf/fsm/and-intersection-or-union to 3c34436

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

2 participants