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 support for manual percentage setting in questions #8890

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

jonatanschroeder
Copy link
Member

@jonatanschroeder jonatanschroeder commented Nov 23, 2023

As discussed in #8858 (comment). This should simplify cases where questions are shared so instructors may keep a consistent ratio of auto to manual points, while still allowing individual assessments to override these values.

In essence, this creates columns for manual percentage in questions and assessment questions, as well as in the JSON schemas. At this point this retains the behaviour of manual points and auto points in the grading process, i.e., #8858 (and #8899) remain open for now.

A future separate PR should populate these values where necessary, based on the grading method (question) and manual points / auto points (assessment question).

@jonatanschroeder
Copy link
Member Author

Also pending: a question and assessment in example course to show the behaviour.

Copy link
Member

@mwest1066 mwest1066 left a comment

Choose a reason for hiding this comment

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

I like this PR in general but I'm wondering about a more comprehensive implementation which:

  1. Deletes the assessment_questions.max_auto_points and max_manual_points columns.
  2. Add a assessment_questions.manual_perc column (may be NULL, meaning use the default from the question).
  3. Changes all code to use manual_perc, points, points_list, and max_points. See Review handling of percentage-based input in manual grading #8858 (comment) for some thoughts on details of this.

To implement this I think we'd probably need a multi-stage deploy, which first adds the new columns and code to update the new columns, then backfills data, then switches the code to use the new columns and deletes the old ones.

In general I'd like to deprecate the existing schema properties and corresponding columns and code for auto points and manual points, to keep things clean. We can still retain compatibility with the existing schema properties by reading them at sync time.

apps/prairielearn/src/schemas/schemas/infoQuestion.json Outdated Show resolved Hide resolved
database/tables/questions.pg Outdated Show resolved Hide resolved

Auto-grading points are set using the `autoPoints` value. For Exam-type assessments, this option can be set to a single value (in which case a single attempt is allowed), or using an array of values, where each value corresponds to an attempt. For Homework-type assessments, the number of `autoPoints` must be a single value, and it corresponds to the initial value of a correct attempt. Students can attempt the same question again until they get a correct answer and full auto points. Manual grading points are set using the `manualPoints`. It is acceptable to use only one of `autoPoints` or `manualPoints`, in which case the other part of the points will be assigned a value of 0.
- By using `points` to specify the total number of points applied to the question. In this case, the manual points are based on the question's `manualPercentage` setting as a percentage of the total points, and the auto points correspond to the remaining points. If the question does not have an explicit `manualPercentage`, its value is determined by the `gradingMethod` value: 100% if the method is `Manual`, or 0% if the method is `Internal` or `External`.
- By using explicit values for `manualPoints` for manual points and `autoPoints` for auto points. In this case, the total number of points is the sum of these two values. It is acceptable to use only one of `autoPoints` or `manualPoints`, in which case the other part of the points will be assigned a value of 0. To avoid ambiguity, it is an error to use both `points` and `autoPoints`, or `points` and `manualPoints`, in the same question.
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 we should deprecate this option and remove it from the docs. Instead I think we should support manualPerc in infoAssessment.json as well as in infoQuestion.json. I'm happy to do this in a later PR.

@jonatanschroeder
Copy link
Member Author

I like this PR in general but I'm wondering about a more comprehensive implementation which:

  1. Deletes the assessment_questions.max_auto_points and max_manual_points columns.
  2. Add a assessment_questions.manual_perc column (may be NULL, meaning use the default from the question).
  3. Changes all code to use manual_perc, points, points_list, and max_points. See Review handling of percentage-based input in manual grading #8858 (comment) for some thoughts on details of this.

I'm ok with the general approach, though I'm not sold on aq.manual_perc === null meaning to use the default, at least for now. Note that (until #8898 makes any progress) the sync process always updates assessment questions after questions, so it is always possible (for now) to have the manual percentage be updated during the sync process to use the question's value.

BTW, this could also make some parts of the code cleaner. In particular, the parts of the code that check if "this question has manual grading" (or auto grading) have to juggle with special cases like 0 points and preview vs instance question. With the change above, this can be changed to a simple check of the percentage.

To implement this I think we'd probably need a multi-stage deploy, which first adds the new columns and code to update the new columns, then backfills data, then switches the code to use the new columns and deletes the old ones.

I'm fine with that. Which do you prefer:

  1. Have the first stage update both manual_perc and max_manual_points/max_auto_points and keep using the old columns until manual_perc is updated;
  2. Have the first stage already use manual_perc, but queries/middlewares use max_manual_points/max_points if manual_perc is null.

For backfilling data, to make sure it's noted, this would mean updating the questions table based on the grading method, and the assessment question based on the manual points.

In general I'd like to deprecate the existing schema properties and corresponding columns and code for auto points and manual points, to keep things clean. We can still retain compatibility with the existing schema properties by reading them at sync time.

I'm not 100% sold on deprecating the settings. I'd prefer to keep them in the schema, but use them as a basis for computation of the percentage (i.e., not in the database directly). This should allow instructors to have control over, e.g., the proportion of the assessment as a whole that is manually graded. So the instructor has the option of (in the assessment question):

  • Setting only points (and possibly maxPoints for homeworks), which uses the question's default manualPerc value;
  • Setting points (and maybe maxPoints) and manualPerc, which overrides the question's value;
  • Setting autoPoints and/or manualPoints (and maybe maxAutoPoints), which computes the manual percentage based on the ratio of manual points to total points.

That said, cleaning up the options in the docs for clarity is not a bad idea either. In that case, option 3 would be an undocumented feature.

@jonatanschroeder
Copy link
Member Author

@mwest1066 one thing to consider regarding question sharing: say you have a question shared from one course to another. The question has a specified manual percentage, and the assessment that uses that shared question (in a different course) does not have it specified (i.e., it uses the default percentage). Now, presumably, changes in the question settings apply across the courses that use it. What should happen if the percentage changes? Does the assessment take the new percentage? What if:

  1. the assessment didn't start yet?
  2. the assessment started, and there are submissions with auto-score, but no manual score?
  3. the assessment started, and manual grading is done?
  4. the assessment started, and manual grading is partially done?

@jonatanschroeder jonatanschroeder marked this pull request as draft November 29, 2023 19:10
@jonatanschroeder
Copy link
Member Author

@mwest1066 one thing to consider regarding question sharing: say you have a question shared from one course to another. The question has a specified manual percentage, and the assessment that uses that shared question (in a different course) does not have it specified (i.e., it uses the default percentage). Now, presumably, changes in the question settings apply across the courses that use it. What should happen if the percentage changes? Does the assessment take the new percentage? What if:

  1. the assessment didn't start yet?
  2. the assessment started, and there are submissions with auto-score, but no manual score?
  3. the assessment started, and manual grading is done?
  4. the assessment started, and manual grading is partially done?

This has been set now to update the manual/auto points on assessment sync, while still updating max_manual_points and max_auto_points. Once the values of manual_perc have been populated in all questions and assessment questions in the DB, these columns can be deprecated and eventually removed, and in that case the manual percentage will be retrieved during the grading process, which in effect means that it will be updated on assessment sync if the assessment has an explicit value, and on question sync (of the question course) if the assessment does not have an explicit value.

@mwest1066 mwest1066 self-assigned this Feb 8, 2024
Copy link
Contributor

github-actions bot commented Mar 21, 2024

All images

Image Platform Old Size New Size Change
prairielearn/executor:19cdc1ccac9b5d0a734fe74390885ad6d7fd731b null 1635.48 MB 1635.66 MB 0.01%
prairielearn/prairielearn:19cdc1ccac9b5d0a734fe74390885ad6d7fd731b linux/amd64 1635.47 MB 1635.66 MB 0.01%

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.

None yet

2 participants