[#38] Add ActivationStrategy and context ranking implementation #51

Denire · 2020-09-21T23:35:41Z

This pull request adds ActivationStrategy interface and two implementations:

Default implementation - ActivationByConfidence - works like it was working before.
Ranking by context implementation - ActivationByContextPenalty.

Resolves #38

- add ActivationStrategy.kt - add ActivationByConfidence.kt as default implementation (as this strategy was used by default in framework) - implement ActivationByContextPenalty.kt strategy, adding penalty by context transitions - add basic tests - ActivationByContextPenaltyTest.kt

nikvoloshin

@morfeusys, @Denire As far as I remember, we discussed some days ago that transition managing can be moved from Activator to BotEngine or somewhere. The point of making Activators responsible for transition managing was the fact that different activator implementations can use different ranking mechanisms. Now, this responsibility is moved to ActivationStrategy, so why not to extract transition managing from Activator?

core/src/main/kotlin/com/justai/jaicf/model/activation/ActivationStrategy.kt

core/src/main/kotlin/com/justai/jaicf/activator/strategy/ActivationByConfidence.kt

core/src/main/kotlin/com/justai/jaicf/activator/strategy/ActivationByContextPenalty.kt

nikvoloshin · 2020-09-22T10:33:43Z

core/src/main/kotlin/com/justai/jaicf/activator/strategy/ActivationByContextPenalty.kt

+     * @return 1 - x - x/2 - x/3 - x/4 - ... x/(l+1)
+     */
+    private fun calculatePenalty(similarityLevel: Int) =
+        (0 until similarityLevel).fold(1.0) { penalty, level ->


Can be simplified to return 1 - stepUpPenaltyBase * (1..similarityLevel).map { 1.0 / it }.sum()

nikvoloshin · 2020-09-22T10:37:27Z

core/src/test/kotlin/com/justai/jaicf/core/test/strategy/ActivationByContextPenaltyTest.kt

+    private val bc = BotContext("", DialogContext().apply { currentContext = currentNode })
+
+    @Test
+    fun shouldRankStrictActivations() {


Let's write more proper test names using backticks

It looks better if a function name in backticks starts with a capital letter

Denire · 2020-09-22T12:17:14Z

@morfeusys, @Denire As far as I remember, we discussed some days ago that transition managing can be moved from Activator to BotEngine or somewhere. The point of making Activators responsible for transition managing was the fact that different activator implementations can use different ranking mechanisms. Now, this responsibility is moved to ActivationStrategy, so why not to extract transition managing from Activator?

This responsibility is not completely moved to ActivationStrategy (activationSelector). Some entity still has to generate transitions and create Activation objects from ScenarioModel
I'm not sure that we need to extract this new entity right now, as we don't have any other ScenarioModel even in ideas/issues.

nikvoloshin · 2020-09-22T12:49:36Z

This responsibility is not completely moved to ActivationStrategy (activationSelector). Some entity still has to generate transitions and create Activation objects from ScenarioModel

What are entities that has to generate transitions on their own? After previous changes in activators, there is added a private method BaseActivator.generateTransitions that generates only possible transitions (and therefore it's private). No one custom activator should generate transitions on its own as it may break our internal logic.

I'm not sure that we need to extract this new entity right now, as we don't have any other ScenarioModel even in ideas/issues.

It's not about another ScenarioModel, it's about Separation of Concerns and Single Responsibility. The current scenario model does not limit us in the building of proper architecture where each entity has only one concrete responsibility.

nikvoloshin · 2020-09-22T15:32:40Z

core/src/main/kotlin/com/justai/jaicf/activator/selection/ActivationByConfidence.kt

+     * @see ActivationSelector
+     */
+    override fun selectActivation(botContext: BotContext, activations: List<Activation>): Activation {
+        val first = StatePath.parse(activations.first().state!!)


This implementation doesn't work too.
The previous implementation before changes relied on the fact, that the activations list is pre-sorted and the first element is always on the most relevant layer, that's why there was takeWhile that had taken only elements on the same level that the first is.
So the thing is you cannot rely on the first element now.

Should be fixed now.

And this one doesn't work too, sorry :)
I guess something like that should work, but it needs testing

override fun selectActivation(botContext: BotContext, activations: List<Activation>): Activation { val current = botContext.dialogContext.currentContext val (fromRoot, onPath) = activations.partition { it.state!!.commonPrefixWith(current) == "/" } val sorted = onPath.sortedByDescending { StatePath.parse(it.state!!).components.size } + fromRoot val first = StatePath.parse(sorted.first().state!!) return sorted.takeWhile { StatePath.parse(it.state!!).parent == first.parent }.maxBy { it.context.confidence }!! }

Briefly: we need to sort by context length first, but there can be global activators with a long context, so we split activations on ones that accessible directly from the root and ones that accessible from some state on the path from the current state to the root, and then sort adding all "from the root" states to the end.

morfeusys · 2020-09-24T08:40:50Z

core/src/main/kotlin/com/justai/jaicf/model/activation/ActivationSelector.kt

+     *
+     * @return number of transitions between current and target state
+     * */
+    protected fun calculateStatesDifference(targetState: StatePath, currentContext: StatePath): Int {


Why did you leave it protected? Looks like utility function that could be wrapped to companion.

morfeusys · 2020-09-24T08:45:28Z

core/src/main/kotlin/com/justai/jaicf/activator/Activator.kt

     */
    fun activate(
        botContext: BotContext,
-        request: BotRequest
+        request: BotRequest,
+        activationSelector: ActivationSelector


Are you sure that selector should be passed to Activator for each activate invocation? Could it be any case where selector could be changed at runtime?

It doesn't. But I don't like adding ActivationSelector as property to Activator interface. Not every Activator needs activationSelector as property. In fact, Activator implementation can operate without ActivationSelector. It's Activator's implementation decision to use ActivationSelector or not.

Adding ActivationSelector as property means that we should add it to ActivatorFactory, therefore making implementing optional logic even more strict.

So I decided to add activationSelector as argument. UPD: With default value.

nikvoloshin · 2020-09-25T12:58:42Z

core/src/main/kotlin/com/justai/jaicf/model/activation/ActivationSelector.kt

+fun Collection<Activation>.sortedByContext(currentContext: String): List<Activation> {
+    val (fromRoot, onPath) = partition {
+        val isRoot = it.state!!.commonPrefixWith(currentContext) == "/"
+        val isIndirectChild = it.state.asStatePath().isIndirectChildOf(currentContext.asStatePath())


What if an indirect child comes not from the root, but from some of our ancestors? It should have a bigger priority, shouldn't it?
current = /a/b
indirect child = /a/b/c/d, fromState = /a

At this point we don't know if indirect child was be activated by fromState or by global activator, and this issue cannot be solved with current architecture.

Looks like ActivationSelector should start to know about Transitions, should generate sorted transitions with or without penalty. Architectual rework will be done in further commits.

So, we are about to break our current logic (that is, by the way, considered as default in this PR) in order to add some other implementation. I don't think it should be done this way.

No, I meant that we shouldn't merge this PR until I've made changes, so we wont break current logic

Ah okay, I've misunderstood you, sorry.
And about your previous message. I don't think ActivationSelector should be aware of transitions or generate them on its own. We should provide all required data through an Activation object, precisely through some state-like object in it.

I think at some point some entity should take transition management from Activator, i thought about changing this right now, but it leads to way to many changes in architecture.

I think i'll implement your idea with providing Activation with necessary data. Thanks for idea!

@nikvoloshin I've added StatesTransition class holding information about transition (fromState, toState, distance).

But there is much to improve in future. The problems I see now are:

checkStrictTransitions() in BotEngine class. It should not be there, also Strict Transition should be a separate entity.

Distance should be calculated when we create transition (generateTransitions).

I don't like that we have two classes (Transition and StatesTransition (or ActivationTransition?)) containing mostly the same data. Looks like we need only one of them.

But solving any of these problems requires a number of changes unrelated to issue. I'd like to solve them in separate PR in future.

@Denire Thanks for the changes! I like that now Activation has more info about states and transitions.
One thing to discuss: it seems like we don't really need StatesTransition entity. We can pass the Transition object to the Activation and provide all utility functionality as extension functions because StatesTransition is a stateless and immutable object with fromState and toState as primary parameters.
So, please think in this direction, maybe it'd be better just to provide utility extension functions on State and/or Transition instead of creating utility-like entity

@nikvoloshin I thought about it and found two reasons not to use Transition object:

Transition has ActivationRule required property, and in case of Slot Filling or Strict Activations it will require unpleasant changes in BotEngine and other places, which don't care about ActivationRule. Their job is only to create and object and pass it further.
Making ActivationRule optional also doesn't look good.

It may be better to use Transition for ScenarioBuilding and StatesTransition (ActivationTransition?) for processing activations. Therefore we have independent objects, and in case we need to modify one of them, we won't be affecting many classes, which use only ActivationTransition logic or ScenarioBuilding logic

upd: I'll rename StatesTransition to ActivationTransition.

* Add StatesTransition class to contain information about transition (fromState, toState and distance). Added tests to check distance. * Fix bugs when activating fromState activator.

…, add integration tests - renamed StatesTransition to ActivationTransition as this name is most proper - Fixed fromState logic. ActivationTransition now contains state a transition was created from - Added integration tests for Activation

Denire · 2020-09-29T01:28:19Z

@nikvoloshin since last discussion i've created some changes. The most interesting part here is:

    private fun generateTransitions(botContext: BotContext): Map<StatePath, List<Transition>> {
        val currentState = StatePath.parse(botContext.dialogContext.currentContext).resolve(".")
        return generateSequence(currentState) { if (it.toString() == "/") null else it.stepUp() }.map { statePath ->
            val transitions = transitions[statePath.toString()] ?: emptyList()
            statePath to transitions
        }.toMap()
    }

@nikvoloshin , @morfeusys
At the moment when we create an activation, we should know the state a transition was created from. We cannot rely on dialogContext.currentContext as this context is changed whenever we stepUp in states tree. Now every Activation has precise information about Activation Transition: fromState, toState and ActivationContext, therefore user can create a implementation of ActivationSelector.

As we discussed, in some future we'll make architectural changes to Activation, mb add ActivationManager, or some entity to manage activation transitions. But now I decided to make least effort to get the result.

Well, at least when we will make refactoring, we'll have some integration tests now :)

morfeusys · 2020-09-29T09:39:56Z

core/src/main/kotlin/com/justai/jaicf/model/state/ActivationTransition.kt

@@ -1,6 +1,6 @@
 package com.justai.jaicf.model.state

-data class StatesTransition(val fromState: StatePath, val toState: StatePath) {
+data class ActivationTransition(val fromState: StatePath, val toState: StatePath) {


Once fromState and toState are immutable, you could replace functions isToChild() and others with values.

morfeusys · 2020-09-29T09:55:44Z

core/src/main/kotlin/com/justai/jaicf/model/activation/ActivationSelector.kt

@@ -0,0 +1,22 @@
+package com.justai.jaicf.model.activation


Are you sure this interface should be placed to the models' package, not a "selection"?

Denire · 2020-10-05T16:05:59Z

Closed as it was solved in separate pr

Denire added the enhancement New feature or request label Sep 21, 2020

Denire requested review from morfeusys and nikvoloshin September 21, 2020 23:35

Denire self-assigned this Sep 21, 2020

nikvoloshin suggested changes Sep 22, 2020

View reviewed changes

pr fixes: naming, javadocs

d40c5dc

Denire requested a review from nikvoloshin September 22, 2020 15:11

nikvoloshin reviewed Sep 22, 2020

View reviewed changes

Denire added 2 commits September 23, 2020 11:55

fix recognition by confidence

d7aab72

fix activaionSelector: calculate proper states difference

c0a461a

morfeusys requested changes Sep 24, 2020

View reviewed changes

improvement in sorting, ranking and testing

5eceeda

nikvoloshin suggested changes Sep 25, 2020

View reviewed changes

Denire added 2 commits September 28, 2020 09:46

Improvements in activation selection

949967b

* Add StatesTransition class to contain information about transition (fromState, toState and distance). Added tests to check distance. * Fix bugs when activating fromState activator.

morfeusys requested changes Sep 29, 2020

View reviewed changes

Denire closed this Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#38] Add ActivationStrategy and context ranking implementation #51

[#38] Add ActivationStrategy and context ranking implementation #51

Denire commented Sep 21, 2020

nikvoloshin left a comment

nikvoloshin Sep 22, 2020

nikvoloshin Sep 22, 2020

nikvoloshin Sep 22, 2020

Denire commented Sep 22, 2020 •

edited

Loading

nikvoloshin commented Sep 22, 2020

nikvoloshin Sep 22, 2020 •

edited

Loading

Denire Sep 23, 2020

nikvoloshin Sep 23, 2020

morfeusys Sep 24, 2020

morfeusys Sep 24, 2020

Denire Sep 24, 2020

nikvoloshin Sep 25, 2020

Denire Sep 25, 2020

nikvoloshin Sep 25, 2020

Denire Sep 25, 2020

nikvoloshin Sep 25, 2020

Denire Sep 25, 2020

Denire Sep 28, 2020

nikvoloshin Sep 28, 2020 •

edited

Loading

Denire Sep 28, 2020 •

edited

Loading

Denire commented Sep 29, 2020 •

edited

Loading

morfeusys Sep 29, 2020

morfeusys Sep 29, 2020

Denire commented Oct 5, 2020

[#38] Add ActivationStrategy and context ranking implementation #51

[#38] Add ActivationStrategy and context ranking implementation #51

Conversation

Denire commented Sep 21, 2020

nikvoloshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Denire commented Sep 22, 2020 • edited Loading

nikvoloshin commented Sep 22, 2020

nikvoloshin Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikvoloshin Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Denire Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Denire commented Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Denire commented Oct 5, 2020

Denire commented Sep 22, 2020 •

edited

Loading

nikvoloshin Sep 22, 2020 •

edited

Loading

nikvoloshin Sep 28, 2020 •

edited

Loading

Denire Sep 28, 2020 •

edited

Loading

Denire commented Sep 29, 2020 •

edited

Loading