Opened 5 years ago

Closed 5 years ago

#16016 closed defect (fixed)

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

Reported by: skropf Owned by:
Priority: trivial Milestone: sage-6.2
Component: combinatorics Keywords: finite_state_machine
Cc: dkrenn, cheuberg, slabbe Merged in:
Authors: Sara Kropf Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: 3c34436 (Commits) Commit: 3c34436d2329c13d6cbcf198e15642d72b51a8f2
Dependencies: Stopgaps:

Description

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 ticket:15078#comment:32.

Change History (14)

comment:1 Changed 5 years ago by skropf

  • Authors set to Sara Kropf

comment:2 Changed 5 years ago by ncohen

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

comment:3 Changed 5 years ago by skropf

  • Status changed from new to needs_review

comment:4 follow-up: Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_info

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

Nathann

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by 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.

comment:6 Changed 5 years ago by git

  • Commit changed from ee5c29553460b3dac2db0c092433f9a2297c3cff to 3c34436d2329c13d6cbcf198e15642d72b51a8f2

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

3c34436FiniteStateMachine.__add__ is the same as __or__

comment:7 Changed 5 years ago by skropf

  • Status changed from needs_info to needs_review

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

comment:8 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

Oooooooooookayyyyyyyyyyyyyyyyyyyyyyyy !!!

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

Nathann

comment:9 follow-up: Changed 5 years ago by ncohen

  • Status changed from positive_review to needs_work

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

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

comment:10 in reply to: ↑ 5 Changed 5 years ago by slabbe

Replying to 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

comment:11 Changed 5 years ago by ncohen

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

Nathann

comment:12 in reply to: ↑ 9 Changed 5 years ago by skropf

Replying to ncohen:

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?

comment:13 Changed 5 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_work to positive_review

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

Nathann

comment:14 Changed 5 years ago by vbraun

  • Branch changed from u/skropf/fsm/and-intersection-or-union to 3c34436d2329c13d6cbcf198e15642d72b51a8f2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.