LearnLib / automatalib

A free, open-source Java library for modeling automata, graphs, and transition systems
http://automatalib.net
Apache License 2.0
92 stars 34 forks source link

CompactMealyTransition does not extend MealyTransition #24

Closed dhendriks closed 6 years ago

dhendriks commented 6 years ago

Both CompactMealyTransition and MealyTransition have a successor and an output. But CompactMealyTransition does not extend MealyTransition, which means transitions can not be treated in a general way. I think CompactMealyTransition should extend MealyTransition. And maybe other transition classes can benefit from similar changes.

mtf90 commented 6 years ago

I think, the reason why we have a separate CompactMealyTransition class is because it stores the information about the successor id as a primtive int which is always 32 bit. If we'd inherit from the generic MealyTransition class (and bind its state generic to Integer) we'd have to store an object reference (64bit on 64bit systems) and an additional object on the heap (i think by default Java caches only Integers from -128 to 127). So it would be contrary to its purpose of being a compact transition representation.

However, I see you point that it may be inconvenient to have incompatible classes for the same concept (e.g. FastMealies and CompactMealies don't share a transition type). We could look into declearing a MealyTransition interface and simply have both implementations coexist under the same super type. Do you have a specific use-case where you encountered this issue? This would help to gauge the scope of this refactoring.

dhendriks commented 6 years ago

I think indeed if you make MealyTransition an interface, then for CompactMealyTransition you would have to implement the 'getSuccessor' method that returns an Integer, but you can still implement 'getSuccId' as you do now and return the 'int'. For implementing an interface you can choose your own storage, as you don't have to call the super constructor and provide the Integer there.

I've since found another way to do what I originally wanted to do, so for me it is not an issue anymore. But I still think in the general design it would be an improvement.

mtf90 commented 6 years ago

I've been experimenting with some approaches to address this issue, but the more I think about it, the less I feel this is a good idea / actually needed. Either you force an unnecessary constraint on Mealy implementations (if you want to do something like MealyMachine<S, I, T extends MealyTransition<S, O>, O>) or the additional interface is just another layer without any real benefit.

The information you are interested in (the transition successor and transition output) can be acquired from the MealyMachine itself, via its

methods. So each implementation can very specifically handle its own transition type. However, transitions can still be treated generally, as it is just some (often unknown) generic type. This is why it would be interesting to look at your use-case, to see if there is a real benefit to a common super type, or if it can also be solved elegantly with the current implementation.

I'm even at a point where I would say the CompactMealyTransition could be replaced by an even more compact implementation (thus advocating the current generic approach).

dhendriks commented 6 years ago

Indeed I was trying to get a generic signature in place that accepts any Mealy machine, while still being able to get the successor and output. Since Mealy transitions do not share a common interface or base class, I created this issue. As I mentioned, I found another way, which indeed is the two methods you mention, that can be invoked on the Mealy machine itself. As such, I agree that it is not actually needed to have a common interface or base class in this case.

mtf90 commented 6 years ago

Ok, great. I can see the appeal of having the output/successor right at the transition object (such with MealyTransitions). But that would force every implementation to expose these information via the transition type, whereas as some (compact) implementations may just use an Integer to represent transitions.

But thank you for raising this issue. It pointed me towards overthinking (and hopefully improving) the CompactMealy implementation in the future.