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: Make algorithm score setting consistent with presence of model solutions #1585

Merged
merged 7 commits into from
Feb 28, 2024

Conversation

faucomte97
Copy link
Contributor

@faucomte97 faucomte97 commented Feb 22, 2024

The Level model has many fields, among which are the following:

  • disable_algorithm_score, set to True if we want the level not to have an algorithm score (therefore, max score is 10).
  • model_solution. Badly named, this field is a string which represents a list of integers, each integer in the list represents how many blocks can be used in the solution of the level to get 10/10 algorithm score. Eg, if model_solution is [5, 6, 8], then the player will get 10/10 in algorithm score for using a solution which has either 5, 6 or 8 blocks in it. Most levels have only one integer in this list.

There are a few considerations to be done here:

  • The model_solution field applies only to levels which use Blockly. Any Python-only levels should have a blank model_solution field (currently represented by an empty list string).
  • The point of the model_solution field is to allow for the computation of the algorithm score. As such, levels for which disable_algorithm_score is True should also have a "blank" model_solution.

The problem is that these constraints aren't enforced anywhere, as such we had two main issues:

  • Some Blockly levels did not have a populated model_solution field: this meant that these levels only had a score out of 10 when in fact they should've been out of 20. This manifested in the form of the "full score" star not appearing on the scoreboard for these levels, as the logic was expecting a score out of 20.
  • Some levels which had no model_solution (most of them Python only levels) still had the algorithm score enabled. This is non-sensical since the algorithm score computation requires a) Blockly and b) a model_solution and Python only levels don't have either. This is a symptom of the fact that we still need to implement an algorithm score for Python levels. For this issue, this manifested itself as scores showing 20/10 in the game.

Main fixes:

  • Added a migration (0090) which populates the model_solution field for the levels that were missing it (these were only the levels in Episode 10).
  • Added a migration (0091) which disables the algorithm score for all levels without a model_solution.
  • Commented out the "hack" in game.js which doubled the total score in Python levels to force them to be 20 even though the level itself is out of 10.
  • Removed the additional check in pathFinder.js which checked for the presence of model_solution in order to compute the algorithm score - now redundant as the code can only check whether the level has the algorithm score enabled directly or not.

This change is Reviewable

@faucomte97 faucomte97 self-assigned this Feb 22, 2024
@faucomte97 faucomte97 changed the title fix: Set Python scores to 10 fix: Make algorithm score setting consistent with presence of model solutions Feb 23, 2024
Copy link
Contributor

@SKairinos SKairinos left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faucomte97)


game/migrations/0090_add_missing_model_solutions.py line 24 at r1 (raw file):

    for level_name, model_solution in model_solutions.items():
        level = Level.objects.get(name=level_name)

don't pull the objects into memory. you don't need to read - only update. instead:

count = Level.objects.filter(name=level_name).update(model_solution=model_solution)
assert count == 1

game/migrations/0090_add_missing_model_solutions.py line 30 at r1 (raw file):

    Attempt = apps.get_model("game", "Attempt")

    Attempt.objects.filter(level__episode__pk=10, score=10).update(score=20)

rather than giving the students' attempt 20 by default, should we not check if their attempt contains the required number of blocks in the solution?

I have a follow up question: how do we calculate the score if it's less/more than a model solution? let's discuss on a call.


game/migrations/0090_add_missing_model_solutions.py line 37 at r1 (raw file):

    episode_10 = Episode.objects.get(pk=10)

    episode_10.level_set.all().update(model_solution="[]")

This code is dangerous because it assumes the queryset is the same as the forward migration. should be explicit:

Level.objects.filter(name__in=["80", "81", "82", "83", "84", "85", "86", "87", "88", "89", "90", "91"]).update(model_solution="[]")


game/migrations/0091_disable_algo_score_if_no_model_solution.py line 15 at r1 (raw file):

    Attempt.objects.filter(
        level__default=True, level__model_solution=False, score=20

why level__model_solution=False? isn't model_solution a char field? did you mean level__model_solution=""?

Copy link
Contributor

@SKairinos SKairinos left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97)


game/migrations/0090_add_missing_model_solutions.py line 51 at r2 (raw file):

    )

    for attempt in attempts:

Dangerous to iterate a large queryset - this could cause an out-of-memory exception.

You'll need to use an iterator: https://docs.djangoproject.com/en/5.0/ref/models/querysets/#iterator

Your solution will need to incorporate:

  • iterator()
  • prefetch_related() since we know we need attempt.level
  • chunk_size

Copy link
Contributor Author

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @SKairinos)


game/migrations/0090_add_missing_model_solutions.py line 24 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

don't pull the objects into memory. you don't need to read - only update. instead:

count = Level.objects.filter(name=level_name).update(model_solution=model_solution)
assert count == 1

Done.


game/migrations/0090_add_missing_model_solutions.py line 30 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

rather than giving the students' attempt 20 by default, should we not check if their attempt contains the required number of blocks in the solution?

I have a follow up question: how do we calculate the score if it's less/more than a model solution? let's discuss on a call.

Done. Bit worried again about memory though.


game/migrations/0090_add_missing_model_solutions.py line 37 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

This code is dangerous because it assumes the queryset is the same as the forward migration. should be explicit:

Level.objects.filter(name__in=["80", "81", "82", "83", "84", "85", "86", "87", "88", "89", "90", "91"]).update(model_solution="[]")

Done.


game/migrations/0090_add_missing_model_solutions.py line 51 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Dangerous to iterate a large queryset - this could cause an out-of-memory exception.

You'll need to use an iterator: https://docs.djangoproject.com/en/5.0/ref/models/querysets/#iterator

Your solution will need to incorporate:

  • iterator()
  • prefetch_related() since we know we need attempt.level
  • chunk_size

Done.


game/migrations/0091_disable_algo_score_if_no_model_solution.py line 15 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

why level__model_solution=False? isn't model_solution a char field? did you mean level__model_solution=""?

Done.

Copy link
Contributor

@SKairinos SKairinos left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faucomte97)


game/migrations/0090_add_missing_model_solutions.py line 30 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Done. Bit worried again about memory though.

what is the scenario you're worried about?

Copy link
Contributor Author

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SKairinos)


game/migrations/0090_add_missing_model_solutions.py line 30 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

what is the scenario you're worried about?

no all good now, since we fixed it with the iterator

Copy link
Contributor

@SKairinos SKairinos left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)

@faucomte97 faucomte97 merged commit bc244c3 into master Feb 28, 2024
5 of 7 checks passed
@faucomte97 faucomte97 deleted the fix_scoring branch February 28, 2024 11:38
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.

2 participants