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

Distribute.py script which generates a ta_list.yml file for use by download.py #42

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

Conversation

Cavtheman
Copy link
Contributor

The implementation currently has two modes:

  • 'Split all': splits all submissions evenly between a given number of TAs, from all student in a course
  • 'By Section': splits by section, with an optional --balance flag to redistribute submissions to even the workload

Known bugs:

  • Group submissions in which groups have members from different sections are duplicated, and in some cases not all members are added
  • The --balance flag can cause duplicates in group assignments, since groups can be split up. The script doesn't currently consider groups at all.

Potential improvements:

  • Allow redistribution to fewer/more TAs than there are sections. Currently when splitting by section, it splits into as many groups as there are sections, with no regard for the number of TAs.
  • Improve the balancing algorithm such that it is not possible for a section with too many submissions to get submissions from other sections. This can happen currently, though the result is still balanced.
  • Support for group assignments.

@kfl
Copy link
Owner

kfl commented Feb 24, 2022

Thanks a lot for your PR.

However, I'll not merge this PR in its current state:

  • The git history is a mess with old commits from your previous PR and a merge commit. I suggest that you either make a feature branch or rebase your fork.
  • Your code does not pass the CI checks. (Currently there is really just a mypy check. I should probably add some tests as well.)
  • The README should be updated with some documentation.
  • I'm not super keen on merging it with known bugs (unless these are properly documented as limitations. It would also be nice to be able to detect when errors did/will occur.)

If you have difficulties fixing any of these points, feel free to either ask questions here or drop by my office.

It seems that staffeli_nt might have a longer lifespan than the two weeks I originally intended it to have. Thus, I want to maintain a minimal repo hygiene.

By the way, may I ask for which course you are using it?

@Cavtheman
Copy link
Contributor Author

I've convinced the professor and other TAs to use it in Elements of Machine Learning. I know Sofie has also convinced the TAs in Softwareudvikling to use it

@Cavtheman
Copy link
Contributor Author

I have made a feature branch named "distribute", but I don't think I can change the base of the pull request? I think that it should clean up the commit log.

@kfl
Copy link
Owner

kfl commented Mar 24, 2022

It seems that you have added the --no-unzip code to this PR as well.

@kfl
Copy link
Owner

kfl commented Mar 24, 2022

I have made a feature branch named "distribute", but I don't think I can change the base of the pull request? I think that it should clean up the commit log.

I think you have to make a new PR if you want to change branch. A PR literally means a Request to Pull from a specific branch.

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.

2 participants