Adds diagnostic test engine service #16453

Nik-09 · 2022-10-31T14:05:24Z

Overview

The PR adds an engine to maintain the state and run the diagnostic test.

Essential Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The linter/Karma presubmit checks have passed locally on your machine.
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
- This lets reviewers restart your CircleCI tests for you.
The PR is made from a branch that's not called "develop".

Proof that changes are correct

The PR does not contain any UI changes, hence UI proof is not required.

PR Pointers

Make sure to follow the instructions for making a code change.
If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
For what code owners will expect, see the Code Owner's wiki page.
Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
Never force push. If you do, your PR will be closed.
Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

…nto diagnostic_player

…tic_player

…tic_test_algorithm

…nto diagnostic_test_algorithm

…tic_test_algorithm

Rijuta-s

A few minor comments. PTAL

Rijuta-s · 2022-11-10T21:20:45Z

core/templates/pages/diagnostic-test-player-page/diagnostic-test-player-status.service.ts

+export class DiagnosticTestPlayerStatusService {
+  private _diagnosticTestPlayerCompletedEventEmitter = (
+    new EventEmitter<string[]>());
+


Suggested change

Not removed, causing lint failures.

Rijuta-s · 2022-11-10T21:21:33Z

core/templates/pages/diagnostic-test-player-page/diagnostic-test-topic-tracker.model.ts

  // and tested in the diagnostic test.
-  _eligibleTopicIds: string[];
+  _pendingTopicIds: string[];


Suggested change

_pendingTopicIds: string[];

private _pendingTopicIds: string[];

Rijuta-s · 2022-11-10T21:23:13Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

+    this._diagnosticTestTopicTrackerModel = diagnosticTestTopicTrackerModel;
+    this._initialCopyOfTopicTrackerModel = cloneDeep(
+      diagnosticTestTopicTrackerModel);
+


Suggested change

Rijuta-s · 2022-11-10T21:24:53Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

+    this.questionBackendApiService.fetchDiagnosticTestQuestionsAsync(
+      this._currentTopicId, this._encounteredQuestionIds).then((response) => {
+      this._diagnosticTestCurrentTopicStatusModel = (
+        new DiagnosticTestCurrentTopicStatusModel(response));
+      this._currentSkillId = (
+        this._diagnosticTestCurrentTopicStatusModel.getNextSkill());
+      this._currentQuestion = (
+        this._diagnosticTestCurrentTopicStatusModel.getNextQuestion(
+          this._currentSkillId)


What will happen if fetch request fails? Should not we also catch the error response of the request?

Added reject handler

Rijuta-s · 2022-11-10T21:26:19Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

+      this.questionBackendApiService.fetchDiagnosticTestQuestionsAsync(
+        this._currentTopicId, this._encounteredQuestionIds).then(
+        skillIdToQuestionsModel => {
+          this._diagnosticTestCurrentTopicStatusModel = (
+            new DiagnosticTestCurrentTopicStatusModel(
+              skillIdToQuestionsModel)
+          );
+          this._currentSkillId = (
+            this._diagnosticTestCurrentTopicStatusModel.getNextSkill());
+
+          this._currentQuestion = (
+            this._diagnosticTestCurrentTopicStatusModel.getNextQuestion(
+              this._currentSkillId)
+          );
+          return successCallback(this._currentQuestion);
+        }
+      );


Ditto as above

oppiabot · 2022-11-10T21:32:46Z

Unassigning @Rijuta-s since the review is done.

oppiabot · 2022-11-10T21:32:49Z

Hi @Nik-09, it looks like some changes were requested on this pull request by @Rijuta-s. PTAL. Thanks!

seanlip · 2022-11-12T02:57:54Z

core/templates/pages/diagnostic-test-player-page/diagnostic-test-topic-tracker.model.ts

  // and tested in the diagnostic test.
-  _eligibleTopicIds: string[];
+  _pendingTopicIds: string[];


Shouldn't it be pendingTopicIdsToTest (and similarly elsewhere)? I think just "pending" is too general.

seanlip · 2022-11-12T02:58:17Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

    let rootTopicIds: string[] = this.getRootTopicIds();
    let failedRootTopicIds: string[] = rootTopicIds.filter(
      topicId => failedTopicIds.indexOf(topicId) !== -1
    );

    if (failedRootTopicIds.length >= 2) {
+      // If the learner failed in two or more root topics, then any two root
+      // topics were recommended, so that learner can start with any


... then we recommend any two root topics, so that the learner can start with any one of them.

seanlip · 2022-11-12T02:58:58Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

      recommendedTopicIds.push(failedRootTopicIds[0]);
    } else {
-      let sortedTopicIds = this.getSortedTopicIds();
+      // If none of the failed topics is a root topic, then the failed topics
+      // are topologically sorted and then the first topic among the sorted list


...then we sort the failed topics topologically and recommend the first topic from the sorted list.

seanlip · 2022-11-12T03:00:07Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

@@ -260,6 +267,8 @@ export class DiagnosticTestPlayerEngineService {
  }

  getRootTopicIds(): string[] {
+    // The topics which do not contain any prerequisites are referred to as


This comment should go above a line that has "root topics" in it -- so, either above the function name, or above "let rootTopicIds".

You can also use JSDoc if you like.

seanlip · 2022-11-12T03:01:29Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

+      // If none of the failed topics is a root topic, then the failed topics
+      // are topologically sorted and then the first topic among the sorted list
+      // will be recommended.
+      let sortedTopicIds = this.getSortedInitialTopicIds();


I don't know what "sorted initial topic IDs" means. Couldn't you just say getTopologicallySortedTopicIds()?

But also, I'm not sure which topic IDs you are referring to -- how does this method know which topics are failed? Shouldn't we pass in a list of failed topic IDs to it so that it can topologically sort those? The word "initial" doesn't convey anything to me, and if anything it sounds like the full list of topics which seems different from just the failed ones (as per the comment).

Lastly, I'm not even sure a full top-sort is needed -- couldn't we just compute a list of topic IDs which have no prerequisites within the set?

Updated the name.

This method only sorts the topics. This method will be used internally by the getTopicRecommendations() function.
The recommendation logic:

Check if the failed topics have any root topics.

If 2 or more roots are present in the failed topics: Recommend any two roots.

If 1 root topic is failed: Recommend the root topic.

If no root topics then,

Sort the topic IDs that were initially present i.e., topicIdToPrerequisiteTopicIds. Here I am sorting all the topics not only the failed ones because the failed topics may or may not have dependencies with each other.

After sorting I am picking the first topic from the sorted list that is failed by the learner.

Answering the last point: Can you please confirm that in the last para, are you referring to the root topic IDs (i.e., compute a list of root topics, because they have no prerequisites)?
If yes: The failed topic IDs may or may not contains the root topics, if they contain any root topic, then the recommendation function will take care of that. Also, we only need to sort the topics if the failed topics list does not contain any of the root topics.
If not: Please help me to understand the last point.

Thank you

[Other comments are addressed locally, currently pushing the changes. I will reassign it for review once the changes will be pushed]

Hi @Nik-09, I almost missed this! Please remember to reassign to whoever needs to take the next action, even though it might just be answering a question.

For the last point, I was talking about looking for topics within the failed topics that do not have prerequisites within the failed topics. I.e. consider the nodes that consist of the failed topics only. Within that graph, we just want to return the topics that have no other prerequisites, right?

E.g. if you have A --> B, B --> C, A --> D, D --> C, C --> E and only B, D, C, E are failed, then B and D are root topics in the "failed graph" and the ones that would be recommended. C has prerequisites B/D and E has prerequisite C, so they're not eligible. Although B and D have A as a prerequisite, A isn't a failed topic and isn't in the smaller "failed graph".

Does this clarify?

Yes, I got the point, but originally we have a dict containing all the topicIdToPrerequisite topic IDs.

In the end, when we have to sort the failed topic ID, we just need a dependency graph for that we have to remove the non-failed topics from the original dependency graph, which may be extra.

Now in order to minimize the full topological sorting I think terminating the function if we encounter the first failed ID while traversing the full graph may improve the efficiency.

So based on that, the new method name can be: getTopologicallyFirstTopicId()
The method returns the first topic ID from the topologically sorted list of the given topic IDs.

Can you please provide feedback on this approach?

I think something like that is totally fine. Not sure if you want to return 1 or 2 IDs but the basic idea makes sense to me!

seanlip · 2022-11-12T03:03:27Z

@Nik-09 Reviewed and replied -- PTAL.

kevintab95

Thanks @Nik-09! I have the same comments as other reviewers in the codeowner files. Please address them. I'm approving to unblock.

oppiabot · 2022-11-12T10:59:59Z

Unassigning @kevintab95 since the review is done.

heyimShivam

LGTM

heyimShivam · 2022-11-12T11:00:52Z

core/templates/pages/diagnostic-test-player-page/diagnostic-test-player-status.service.ts

+
+

Suggested change

heyimShivam · 2022-11-12T11:02:10Z

core/templates/pages/exploration-player-page/services/diagnostic-test-player-engine.service.ts

+
+

Suggested change

oppiabot · 2022-11-12T11:05:49Z

Unassigning @heyimShivam since they have already approved the PR.

Nik-09

Hi, @seanlip @Rijuta-s I have addressed the open comments. Please take a look once you are free.
Thank you

seanlip

Thanks @Nik-09, LGTM. On balance I think you can skip the "failed topic prereqs" optimization -- since the graph is only 15 nodes and this calculation is just run once, it probably won't make much difference anyway.

Feel free to merge when you're ready!

…tic_test_engine_service

Rijuta-s

Thanks @Nik-09 ! LGTM!

Nik-09 added 30 commits October 10, 2022 22:32

Adds diagnostic test player page

3cc8a69

Adds html text

d0accc0

Merge branch 'diagnostic_player' of https://github.com/Nik-09/oppia i…

9efdabf

…nto diagnostic_player

Adds frontend unit tests

f0dd1a6

Adds backend tests

02bdc42

Fixes linter issue

7544164

Fixes mypy issues

6023501

Fixes typescript checks

95c0d30

Updates copyright text.

3faeeb8

Merge branch 'develop' of https://github.com/oppia/oppia into diagnos…

2eca459

…tic_player

Adds diagnostic test model

6abd20b

Alignes diagnostic test player with the question player

4b4175b

Merge branch 'develop' of https://github.com/oppia/oppia into diagnos…

6aa8064

…tic_player

Internationalized diagnostic test texts

793e1d4

Adds css rules

b18e16e

Adds Qqq text.

a12514b

Adds frontend unit tests

9711984

Fixes lint issues

1e40d37

Adds test file to the backend shard.

3ef4c8a

Adds unit test

8236fbe

Fixes lint issues

d5e8301

Fixes lint issues

32df356

Fixes typescript issue.

84c0a0d

Fixes merge conflict with the diagnostic player

c8cc619

Merge branch 'develop' of https://github.com/oppia/oppia into diagnos…

c4d8add

…tic_player

Sort i18n keys

28fe324

Merge branch 'develop' of https://github.com/oppia/oppia into diagnos…

655fb55

…tic_test_algorithm

Adds frontend unit test for the diagnostic test model.

efd6ef9

Merge branch 'diagnostic_player' of https://github.com/Nik-09/oppia i…

e2235b2

…nto diagnostic_test_algorithm

Merge branch 'develop' of https://github.com/oppia/oppia into diagnos…

405e2aa

…tic_test_algorithm

Nik-09 assigned seanlip, kevintab95 and heyimShivam and unassigned Nik-09 Nov 10, 2022

Rijuta-s suggested changes Nov 10, 2022

View reviewed changes

oppiabot bot unassigned Rijuta-s Nov 10, 2022

oppiabot bot assigned Nik-09 Nov 10, 2022

seanlip requested changes Nov 12, 2022

View reviewed changes

seanlip removed their assignment Nov 12, 2022

kevintab95 requested changes Nov 12, 2022

View reviewed changes

kevintab95 approved these changes Nov 12, 2022

View reviewed changes

oppiabot bot unassigned kevintab95 Nov 12, 2022

heyimShivam approved these changes Nov 12, 2022

View reviewed changes

oppiabot bot unassigned heyimShivam Nov 12, 2022

Addresses review comments

06ce94a

Nik-09 assigned seanlip and Rijuta-s and unassigned Nik-09 Nov 12, 2022

Nik-09 commented Nov 12, 2022

View reviewed changes

seanlip approved these changes Nov 12, 2022

View reviewed changes

seanlip removed their assignment Nov 12, 2022

Nik-09 added 2 commits November 13, 2022 08:20

Removes blank lines

51cd929

Merge branch 'develop' of https://github.com/oppia/oppia into diagnos…

e6d78a3

…tic_test_engine_service

Rijuta-s approved these changes Nov 13, 2022

View reviewed changes

Rijuta-s merged commit f8a8d68 into oppia:develop Nov 13, 2022

	_pendingTopicIds: string[];
	private _pendingTopicIds: string[];

Adds diagnostic test engine service #16453

Adds diagnostic test engine service #16453

Conversation

Nik-09 commented Oct 31, 2022 • edited Loading

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

Rijuta-s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nik-09 Nov 12, 2022 • 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

oppiabot bot commented Nov 10, 2022

oppiabot bot commented Nov 10, 2022

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

Nik-09 Nov 12, 2022 • 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

seanlip commented Nov 12, 2022

kevintab95 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 12, 2022

heyimShivam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 12, 2022

Nik-09 left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Rijuta-s left a comment

Choose a reason for hiding this comment

Nik-09 commented Oct 31, 2022 •

edited

Loading

Nik-09 Nov 12, 2022 •

edited

Loading

Nik-09 Nov 12, 2022 •

edited

Loading