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

Slip Calculator Jobs for Assignment and Course #1148

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

epai
Copy link
Contributor

@epai epai commented May 5, 2017

Slip Calculator Jobs (Resolves #1068)

This helps courses to:

  1. Calculate Slips for a particular assignment.
  2. Calculate Slips for all completed assignments for a given course.

Calculate Course Slips

snip20170505_89
snip20170505_87
snip20170505_95

Calculate Assign Slips

snip20170505_91
snip20170505_93
snip20170505_94

@epai epai requested a review from Sumukh May 5, 2017 09:02
@Sumukh
Copy link
Member

Sumukh commented May 5, 2017

Haven't looked at the code but going off the screenshot.

I think the slip days an action from the course dashboard - not from the assignments

I'm not so sure about the wording of "Calcuate Slips" - Late Submission Report or Even just saying grace period"

@Sumukh Sumukh closed this May 5, 2017
@Sumukh Sumukh reopened this May 5, 2017
@epai
Copy link
Contributor Author

epai commented May 5, 2017

Sumukh, Thanks for the quick response and feedback!

Where on the course dashboard were you thinking? I experimented with putting the slip action in the course dashboard quicklinks, but decided against it because it doesn't fit the pattern of the rest of the links (all other links link to some view, but the slip action would start a job). We could add a section to the course dashboard specifically for actions. The reason why I put it on the assignments page was I thought it made most sense because calculating slips is an action on assignments. I'm open suggestions, though.

I agree the name calculate slips is a bit misguided, because we're not actually applying slip days or whatnot, just showing who turned in things late and how late they were. I like the late submissions report naming better - shall I rename everything to fit this naming (e.g. slip days used -> days late)?

@knrafto
Copy link
Contributor

knrafto commented May 5, 2017

I agree that "slip days" is misleading. I would expect OK to deal with grade changes in that case.

I'm curious how instructors will use this. Will they manually go through all entries in the CSV and give credit to late assignments?

@Sumukh
Copy link
Member

Sumukh commented May 5, 2017

I'm curious how instructors will use this. Will they manually go through all entries in the CSV and give credit to late assignments?

Data 100 for example just wants calculate slip day usage (Everyone is given X slip days) - CSV is a good workflow since they aren't managing all their grades in OK anyway.

@Sumukh
Copy link
Member

Sumukh commented May 5, 2017

Other notes:

  • Can be useful to have the SID too (and the name)
  • CSV should be a download
  • Let's just not display the results in the log
  • There was some sort of error in staging worth debugging.


class AssignSlipCalculatorForm(BaseForm):
timescale = SelectField('Time Scale', default="days",
choices=[(c.lower(), c.title()) for c in TIMESCALES],
Copy link
Contributor

Choose a reason for hiding this comment

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

c.lower() seems redundant here, as the TIMESCALES enum is already lowercase

return abort(404)

form = forms.AssignSlipCalculatorForm()
timescale = form.timescale.data.title()
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest keeping the timescale lowercase, so it matches up with the TIMESCALES enum. And you won't need the .lower() on L16 in slips.py

from server.utils import encode_id, local_time, output_csv_iterable
from server.constants import TIMESCALES

timescales = {'days':86400, 'hours':3600, 'minutes':60}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in constants as TIMESCALES, so there's not both TIMESCALES and timescales that have the same keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this originally, but realized if I would then have to use the dictionary keys to fill in the form options, which wouldn't have a guaranteed ordering.

Now that I've thought about it, I think I'll just use an OrderedDict to preserve the ordering of the keys for display on the form.

timescale = SelectField('Time Scale', default="days",
choices=[(c.lower(), c.title()) for c in TIMESCALES],
description="Time scale for slip calculation.")
show_results = BooleanField('Show Results', default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is useful as an option. We should either always show the results, or don't

@@ -236,6 +236,16 @@ def chunks(l, n):
prev_index = index


def output_csv_iterable(header, rows):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too similar to generate_csv below to be another function. It has other problems: it uses Windows line endings, and will buffer the whole thing in memory (which isn't really an issue for these jobs, but it defeats the while purpose of using iterables).

Instead, try to use generate_csv. It would looks something like

    csv = generate_csv(subms, header, get_row)

where subms is the thing you're iterating over, and get_row is a function that returns a dict of CSV values.

@epai
Copy link
Contributor Author

epai commented May 6, 2017

I agree that "slip days" is misleading. I would expect OK to deal with grade changes in that case.

I agree - I was thinking of renaming things "Report Late Submissions" or something similar. If I were to do this, I was thinking of filtering out all non-late submissions and only show late ones.

@colinschoen
Copy link
Member

@epai Are you still planning on working on this one?

@cycomachead
Copy link

At the risk of putting this publicly on GitHub: Most courses only penalize for serious over offenders unless they automate things -- but the automation is slightly tricky as it should apply penalties to the lowest scoring assignment first.

(Well, I suppose with backups the maximal value isn't necessarily the simple algorithm, but I digress.)

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.

Slip Day Calculator Job
5 participants