-
Notifications
You must be signed in to change notification settings - Fork 1
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
Use a Practice ruleset to implement training phases #1360
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and the tests are succeeding. I added some minor questions & comments but nothing all too serious. On a different topic: I had some difficulty finding out what and how to review this PR so maybe it would be a good idea to describe what you are expecting from me in terms of a review so I know how to approach the PR :-)
@@ -7,8 +7,8 @@ | |||
|
|||
class DurationDiscriminationTone(DurationDiscrimination): | |||
ID = 'DURATION_DISCRIMINATION_TONE' | |||
condition = _('tone') | |||
|
|||
subtask = _("Tone") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the translations files (.po | .mo
) be updated after using a "new" translation key or is this translation already present in all languages?
first_condition = "longer" | ||
first_condition_i18n = _("LONGER") | ||
second_condition = "equal" # second condition is 'catch' condition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these texts presented to the user or actual conditions? It confuses me a bit since you are using the translation library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see this is explained in the Practice
class, nvm
from result.utils import prepare_result | ||
from section.models import Playlist | ||
from session.models import Session | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class DurationDiscrimination(Base): | ||
class DurationDiscrimination(Practice): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does inheriting from Practice
mean that this rules file is used solely for practice rounds?
class Practice(Base): | ||
"""Practice is a rules class which presents a trial a given number of times. | ||
At these practice trials, it tests whether the partcipant performed well enough to proceed. | ||
|
||
Use this class as a base for your ruleset if you need a practice phase. | ||
This practice class is now written towards 2 alternative forced choice rulesets, but may be extended in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you maybe describe a mini example of how it differs from the base class? Also, I was thinking: maybe we can call it "WithPractice" to communicate that this is not solely a class meant for practice rounds but merely augments the Base class functionality? Or is this whole class only for Practice blocks?
else: | ||
# generate feedback, then delete all results so far and start over | ||
feedback = self.get_feedback_explainer(session) | ||
session.result_set.all().delete() | ||
return [ | ||
feedback, | ||
self.get_restart_explainer(), | ||
self.get_intro_explainer(), | ||
self.get_practice_explainer(), | ||
self.get_next_trial(session), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else
statement is not necessary per se as you do a return in the if
statement already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even so, I often feel that else
statements increase readability (same indentation for both conditions).
else: | ||
return [ | ||
self.get_feedback_explainer(session), | ||
self.get_next_trial(session), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this else
statement
def next_round(self, session: Session) -> list: | ||
return self.next_practice_round(session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean exactly and what is the difference between next round and next trial action?
Close #92: the rhythm experiments were using a rather awkward setup for their practice phases. Now, they are using a
Practice
ruleset instead, which has some methods that can be overridden to accomplish the desired behaviour.