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

singleResult should score even with messages #524

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

drdrew42
Copy link
Member

@drdrew42 drdrew42 commented Feb 6, 2021

MultiAnswer objects with singleResult=>1 do not properly compute score when setMessage is used.

To test:

DOCUMENT();
loadMacros(
  "PGstandard.pl",
  "PGML.pl",
  "parserMultiAnswer.pl",
);

TEXT(beginproblem());
$ShowPartialCorrectAnswers = 1;

Context("Numeric");

$answerA = MultiAnswer(Real(0), Real(0))->with(
    singleResult=>1,
    checker => sub {
        my ($cor, $stu, $self, $ansHash) = @_;
        $self->setMessage(1, 'message indicating half credit for answer 1 of part A');
        $self->setMessage(2, 'message indicating half credit for answer 2 of part A');
        return 0.75;
    });

$answerB = MultiAnswer(Real(0), Real(0))->with(
    singleResult=>1,
    checker => sub {
        my ($cor, $stu, $self, $ansHash) = @_;
        $self->setMessage(1, 'message indicating half credit for answer 1 of part A');
        return 0.75;
    });

$answerC = MultiAnswer(Real(0), Real(0))->with(
    singleResult=>1,
    checker => sub {
        my ($cor, $stu, $self, $ansHash) = @_;
        # no message about half credit for answer 1 of part C
        return 0.75;
    });

BEGIN_PGML

Single-result Multi-Answer objects are incorrectly scored when setMessage is used.

*Part A*  
Answer A _should_ receive 75% credit and a feedback message for each sub-part.  
1. [__________]{$answerA}  
1. [__________]{$answerA}  

*PartB*  
Answer B _should_ receive 75% credit and a feedback message for just one part.
1. [__________]{$answerB}  
1. [__________]{$answerB}  

*PartC*  
Answer C _should_ receive 75% credit and no feedback message.
1. [__________]{$answerC}  
1. [__________]{$answerC}  

END_PGML
ENDDOCUMENT();

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.

I saw this as well a while back. I thought it was intentional, but didn't like the behavior. Good change.

Copy link
Member

@taniwallach taniwallach left a comment

Choose a reason for hiding this comment

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

Tested and works as expected.
I have been working around this bug in local files for a while by scaling score to counteract the bug... starting from before I was looking at making many code changes...
Although I will need to fix the local problems to remove the hacks - this should certainly be merged as soon as possible.

@drgrice1
Copy link
Member

Merging as soon as possible...

@drgrice1 drgrice1 merged commit be8b0bf into openwebwork:develop Feb 17, 2021
@taniwallach
Copy link
Member

@drgrice1 @drdrew42 - Can we easily condition code in a problem on the PG version in use - so that the "hacks" around the bug could apply if an "old" version of PG was in use? (I should be able to figure this out myself, by maybe someone knows how to do this easily before I try to figure it out.)

@drgrice1
Copy link
Member

@taniwallach: I am not sure. That probably depends on what hacks were used to work around this.

The approach that I use will work the same both with and without this pull request. I don't use the setMessage method, and instead store my messages in a custom key of the answer hash, and then deal with putting those messages into the ans_message variable in a post filter. Part of the reason that I do this is because I don't like the table that the parserMultiAnswer.pl macro creates. It is ugly for one thing. That could be cleaned up with some work on css styling. Furthermore, the "In answer $i" part is generally not as informative as to how the answers match up to which parts as intended in these single result problems. Particularly when errors really relate to multiple parts and how they fit together.

@dpvc
Copy link
Member

dpvc commented Feb 17, 2021

I don't like the table that the parserMultiAnswer.pl macro creates. It is ugly for one thing.

The styling was based on the classes at the time MultiAnswer was first developed (ages ago), and doesn't work so well now. More flexibility in the format of the message would be useful.

This was developed pretty early on, and its various uses and needs were not yet clear at the time. Some updates are probably in order.

@taniwallach
Copy link
Member

@taniwallach: I am not sure. That probably depends on what hacks were used to work around this.

Just a simple multiplication of the desired grade by a factor of the form: N/(N-k) when there are N graded items and k messages were set.
Ex for N = 15; k = 2;
$score *= ( 15 )/( -2 + 15 ) unless $answerHash->{isPreview};

The approach that I use will work the same both with and without this pull request. I don't use the setMessage method, and instead store my messages in a custom key of the answer hash, and then deal with putting those messages into the ans_message variable in a post filter.

That makes more work for the programmer, and others will not know how to do it.

Part of the reason that I do this is because I don't like the table that the parserMultiAnswer.pl macro creates. It is ugly for one thing. That could be cleaned up with some work on css styling.

So let's improve it so others can use the main parserMultiAnswer.pl without needing to know about special post filters.

Furthermore, the "In answer $i" part is generally not as informative as to how the answers match up to which parts as intended in these single result problems. Particularly when errors really relate to multiple parts and how they fit together.

I agree, and in some problems I have put in text explaining about such matters. Maybe we want to be able to have "rows" not linked to a specific box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants