loop_includes_initial flag to Machine.add_ordered_transitions is confusing

[facundofc]

The following code does not work as I would expect:

m = Machine('self', ['A', 'B', 'C'], initial='A')
m.add_ordered_transitions(['C', 'B', 'A'], loop=False)
m.to_C()
m.next_state()

The final next_state() call raises a MachineError complaining that there's no trigger next_state from state B.

Looking at the code, the issue seems to be the loop=False.

My use case is to have an FSM that's just a linear sequence through a set of states, back and forth, with no loop. So my first try didn't work:

m = Machine('self', ['A', 'B', 'C'], initial='A', ordered_transitions=True)
m.add_ordered_transitions(['C', 'B', 'A'], loop=False)

My thought is that what's referred to as the "loop" actually refers to going from the last state to the first of the provided states' list. It could be that the loop_includes_initial flag is meaningless if the first state provided is not the initial one. My proposal is to raise ValueError if loop == True, loop_includes_initial == True but states[0] != self._initial. What do you think about this?

I'm more than willing to submit a pull request about this, but I'm unsure about what the fix should do.

Thanks!

[aleneum]

Hi @facundofc,

this has been open for quite a while and I just had a look at it. Running the code mentioned above works fine for me:

from transitions import Machine

m = Machine(states=['A', 'B', 'C'], initial='A')
m.add_ordered_transitions(['C', 'B', 'A'], loop=False)
m.to_C()
m.next_state()
print(m.state)  # >>> B

Does this issue still occur for you?

Considering your other comments:

It could be that the loop_includes_initial flag is meaningless if the first state provided is not the initial one.

The passed states will be reordered here and the following line to make sure that the initial state is the first argument (introduced due to #268). Right now, this line would cause a ValueError when initial is not part of the passed state array.

My use case is to have an FSM that's just a linear sequence through a set of states, back and forth, with no loop. So my first try didn't work:

m = Machine('self', ['A', 'B', 'C'], initial='A', ordered_transitions=True)
m.add_ordered_transitions(['C', 'B', 'A'], loop=False)

This will cause one event trigger (next_state) having two transitions from the same state. First line will add: A -next_state-> B -next_state-> C -next_state-> A. Second line will add (in this order!): A -next_state-> C -next_state-> B (since loop=False B->A is omitted). When in state A trigger next_state includes: A->B and A->C and since transitions will evaluate transitions in the order they were added, calling next_state in state A will always transition to state B (unless you add conditions).

If you want to transition back and forth I'd suggest two different triggers:

from transitions import Machine

m = Machine(states=['A', 'B', 'C'], initial='A', ordered_transitions=True)
m.add_ordered_transitions(['C', 'B', 'A'], loop=True, trigger='previous_state')
m.next_state()
m.next_state()
m.next_state()
assert m.is_A()
m.previous_state()
m.previous_state()
assert m.is_B()

[facundofc]

Hi @aleneum! Thanks a lot for taking the time to consider my issue.

Does this issue still occur for you?

Well, actually, I apologize since what happened is that I forgot a final m.next_state(). I would expect this to leave me at state A, but that's the call that raises the MachineError exception.

After your comments and some further reading of the code, I think I finally understood exactly what add_ordered_transitions do. Your solution is not good enough for me since it does not generate the set of transitions I want. The transitions I want are precisely: A->B->C on next_state() (btw, I agree on the need for two triggers), and C->B->A on previous_state(). Now, I'd add loop=False to the creation of the machine, but that's ok. On the second line:

m.add_ordered_transitions(['C', 'B', 'A'], loop=True, trigger='previous_state')

loop=True I suppose in order to add the B->A transition, which I want, but it'll also add the A->C transition, which I don't.

I already have a solution to this, which is this:

m = Machine('self', ['A', 'B', 'C'], initial='A', ordered_transitions=True)
m.initial = 'C'
m.add_ordered_transitions(['C', 'B', 'A'], loop=False, trigger='previous_state')

and that works like a charm.

I'd like to make a little digression now, and I hope it's ok with you. I think reordering the states to accomodate for the initial state being the first one is extremely misleading and unexpected. The method's name is add_ordered_transitions, and I would expect it to add the transitions exactly in the order I'm providing the states.

In my opinion, this reordering could be left aside. This is to say that add_ordered_transitions would not consider the initial state at all. This would leave add_ordered_transitions(['e0', 'e1', ..., 'en']) to just add the transitions e0->e1->e2->...->en. The loop parameter would just indicate if en->e0 should be added too or not.

I think this would also have solved #268. This is the code @janekbaraniewski wanted to fix, for easy reference:

machine = Machine(states=states, initial='C')
machine.add_ordered_transitions(loop=False)

assert machine.state == 'C'
machine.next_state()
assert machine.state == 'D'

With my idea, the second line would add all the transitions sequentially, with no loop. Since the machine is already in the C state (because it's set upon creation given the initial='C'), you'd transition to D and the get a MachineError from then on.

Furthermode, I think you can say that the intention of the initial parameter to the machine's constructor is to set a default value for the add_model's initial parameter. You could potentially add different models with different initial states. I consider this to be yet another argument to make add_ordered_transitions leave the initial state alone.

I'm sorry for the long comment. I'd like to know your thoughts on this. I'll try to create a PR soon so that things get more concrete.

Cheers!

[mkaranki]

Hi @aleneum,

The passed states will be reordered here and the following line to make sure that the initial state is the first argument (introduced due to #268). Right now, this line would cause a ValueError when initial is not part of the passed state array.

Is there a need for that ValueError in case the states list does not include the initial state? Could that be avoided e.g. by #392 ?

I agree that the special handling of the initial state is a bit confusing.