Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #114: Implement answer classification controller #211

Merged
merged 21 commits into from
Oct 9, 2019

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Oct 7, 2019

This introduces all of the necessary rule classification support for the prototype, including:

  • Continue
  • TextInput
  • NumericInput
  • NumberWithUnits
  • MultipleChoiceInput
  • ItemSelectionInput
  • FractionInput

This does not include deep tests for any of these classifiers individually (and some may have bugs), but #210 is tracking adding more thorough tests. The AnswerClassificationController is being well tested, and ExplorationProgressController from the base branch runs through real explorations end-to-end which in turn verifies that some of the classifiers are working.

This PR introduces these classifiers in a generic way so that it's easy to add both additional interactions and rule types in the future. It also does generic answer classification in a type-safe way and such that the base logic for the classification is as simple as possible (e.g. verifying strings directly for TextInput rather than extracting the normalized string and verifying that both the input and answer have the correct type).

This also makes a few adjustments in various protos, and fixes a few small issues that were found in ExplorationProgressController.

tests to verify correctness.

Also, added a method to facilitate notifying of DataProvider changes on
the UI thread.
… it up to

the data controller, and start adding tests.
Conflicts:
	domain/build.gradle
	domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt

Also, migrate the data controller to the retriever.
AnswerClassificationController, and attempted to make
ExplorationProgressController thread-safe.

The thread-safety led to significant interface changes in the progress
controller, and led to discovering some issues with the mediator live data
approach to interop coroutines and LiveData. This locking mechanism will
need to change since the optimal solution requires resolving #90.
with the current MediatorLiveData implementation (see #90 for more
context). Fix existing progress controller tests and add a few more. All
current progress controller tests are passing.
…ation

support for the second test exploration (about_oppia).
prototype. This uses a Dagger-powered solution to make it straightforward
to add new rule types and interactions in a way that automatically hooks
into the classifier.

This only adds TextInput support, so existing tests do not pass. Tests and
the app do build.
exploration progress controller tests for numeric input.
comments to point to their corresponding Oppia web versions.
and fix ExplorationProgressController tests (which also included fixing
one bug in the TextInput FuzzyEquals and the progress controller's
fallback routing logic).
@BenHenning BenHenning changed the base branch from develop to introduce-exploration-progress-controller October 7, 2019 08:40
@BenHenning
Copy link
Member Author

Folks: I can split this up, but I'd rather not since it's slightly tricky to do in a way that doesn't cause ExplorationProgressControllerTest to fail. Most of the code is boilerplate, and I actually don't care about you verifying the correctness of the classifiers beyond "does this look like the JavaScript version". Once you skip past those + the Dagger modules, there's not much code left to verify (AnswerClassificationController & ExplorationProgressController + tests are the main chunks of new code).

@BenHenning
Copy link
Member Author

NB: This will be rebased onto develop, so please review as if it's going into develop (because it will be).

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Oct 7, 2019
Conflicts:
	domain/src/main/java/org/oppia/domain/classify/AnswerClassificationController.kt
	domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt
	domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt
	domain/src/test/java/org/oppia/domain/classify/AnswerClassificationControllerTest.kt
	domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt
@BenHenning BenHenning changed the base branch from introduce-exploration-progress-controller to develop October 7, 2019 23:41
@BenHenning
Copy link
Member Author

@vinitamurthi / @jamesxu0 if you get a chance, please review. It'd be good to get a few folks looking at this just due to the amount of stuff being added.

@BenHenning
Copy link
Member Author

Given this is blocking downstream work, I'm going to check this in.

I'm pretty confident in it, but @vinitamurthi & @jamesxu0 please do still take a look at this & let me know if you have any concerns. I'll follow up with a fix in a subsequent PR.

@BenHenning BenHenning merged commit a501b23 into develop Oct 9, 2019
@BenHenning BenHenning deleted the implement-answer-classification-controller branch October 9, 2019 01:38
@BenHenning BenHenning mentioned this pull request Oct 9, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants