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

Add MultiAnswer convenience methods (default checker and passing cmp options). #1140

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

somiaj
Copy link
Contributor

@somiaj somiaj commented Nov 6, 2024

This adds two conveniences to parserMultiAnswer.pl.

  • A cmpOpts option (taken from parserRadioMultiAnswer.pl) to pass a hash of options to all cmp methods.
  • A default checker that is used if no checker is defined. This checker just checks which answer parts are equal, and can either return an array of 0s and 1s for each answer part, or a single score of 0 or 1, depending on if the new option partialCredit is set or not. partialCredit defaults to the value of $showPartialCorrectAnswers.

This is a hash of options that will be passed to the cmp method of all answers. For example,
C<< cmpOpts => { weight => 0.5 } >>. This option is provided to make it more convenient to pass
options to all answers when utilizing PGML. To pass options to a single answer, use the
C<< setCmpFlags($which_rule, %flags) >> method instead. Default: undef (no options are sent).
Copy link
Member

Choose a reason for hiding this comment

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

This documentation is not correct. Regardless of if singleResult is set, this is not a substitute for the setCmpFlags method, and in no case are these flags actually passed to the cmp method for all answers. There are two sets of answer evaluators that a MultiAnswer object uses. There is a set of answer evaluators that are associated with the answer rules (there is only one of these if singleResult is set, and one for each answer otherwise), and there are answer evaluators for each answer that are called internally by the entry_check or single_check methods (there are always as many of these as there are answers regardless of the singleResult setting). The only way to set flags for the internally called answer evaluators you must use the setCmpFlags method. The things set in cmpOpts won't be passed to those evaluators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. I got confused when I thought it was implied at the meeting I could have used that method to add cmp options. I have updated the docs to mirror what you have in parserRadioMultiAnswer.

@somiaj somiaj force-pushed the multi-answer-convenice branch from 1627b2d to 2822f53 Compare November 6, 2024 19:08
foreach my $id ('checker', 'separator') {
if (defined($options{$id})) {
$self->{$id} = $options{$id};
delete $options{$id};
}
}
die "You must supply a checker subroutine" unless ref($self->{checker}) eq 'CODE';

unless (ref($self->{checker}) eq 'CODE') {
Copy link
Member

Choose a reason for hiding this comment

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

You may wish to produce an error message if checker is defined but not a code reference, rather than just replacing it silently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I think you could put the check inside the unless clause, as

unless (ref($self->{checker} eq 'CODE') {
  die "Your checker must be a subroutine." if defined($self->{checker});

instead of having to check for CODE two times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks again.

@somiaj somiaj force-pushed the multi-answer-convenice branch from b3a4b01 to 3a60bf1 Compare November 6, 2024 20:23
Add a convenience option to parserMultiAnswer to send the HASH
of options listed in cmpOpts to all answers cmp methods.
This is the same option copied from parserRadioMultiAnswer.
This adds a simple default checker to parserMultiAnswer that is used
if no checker is provided. The checker checks if each answer part
is correct using the overloaded `==` operator. It can also be configured
to either return which parts are correct, allowing for partial credit,
or only return 0 or 1 depending on if all answer parts are correct.
The `partialCredit` option can be used to control which behavior
to use, and defaults to the value of `$showPartialCorrectAnswers`.

This is just for convince when using MultiAnswer with a simple
checker.
Check if any user defined checker is a subroutine, and give an error
message if not, instead of sliently overwriting it with a default checker.
@somiaj somiaj force-pushed the multi-answer-convenice branch from 3a60bf1 to 29f85de Compare December 10, 2024 19:52
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks good.

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.

3 participants