-
Notifications
You must be signed in to change notification settings - Fork 20
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 conflict handling to random algorithm #277
Conversation
Co-authored-by: Mikey Spurr spurrm@allegheny.edu
Co-authored-by: Mikey Spurr spurrm@allegheny.edu
Co-authored-by: livingstonp729 <livingstonp@allegheny.edu>
Co-authored-by: livingstonp729 <livingstonp@allegheny.edu>
Co-authored-by: livingstonp729 <livingstonp@allegheny.edu>
…p_size function, and more! Co-authored-by: livingstonp729 <livingstonp@allegheny.edu> Co-authored-by: ohnoanarrow <douglasst@allegheny.edu> Co-authored-by: Spurrm <spurrm@allegheny.edu>
Co-authored-by: livingstonp729 <livingstonp@allegheny.edu>
Co-authored-by: Mikey Spurr <spurrm@allegheny.edu>
Co-authored-by: Mikey Spurr <spurrm@allegheny.edu>
Co-authored-by: robert-samuel07 <roberts@allegheny.edu>
Co-authored-by: robert-samuel07 <samuelr@allegheny.edu>
Co-authored-by: robert-samuel07 <samuelr@allegheny.edu>
Co-authored-by: ohnoanarrow <douglasst@allegheny.edu> Co-authored-by: Alex-Yarkosky <yarkoskya@allegheny.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove any trailing comments, such as in group_creation
line 54
. They can make the code harder to read. Instead, comments should go above the lines they document.
@shafferz is this branch now not "currently in progress"? If not, please update the description with all the necessary information. |
Moved trailing comments to precede documenting line of code.
@Michionlion I have made the necessary changes. Let me know what you think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some major problems with this code. First, it is never actually able to run in conflict-avoidance mode (there is no execution path where the conflicts argument is specified). Secondly, there are no tests for the conflicts portion of the code. Third, there are major logical issues that mean that this code almost certainly does not work the way it should (see my review comment).
Because of the fact that this code is not tested or runnable, I cannot merge this PR at this time.
@Alex-Yarkosky @shafferz I'm sorry it took a little while to get to this, but hopefully you will be able to use any remaining time wisely. Had this been pushed and thus reviewed earlier, these problems could have been fixed earlier.
To address the issue of non-access: This code adds the functionality for conflict handling. The teams responsible for working on the front end and the database would need to have been the ones to add the ability of conflict reception to use the functions as we have written them. Simply put, our team had absolutely no access or ability to develop on the website to implement this feature on that end. This is strictly a back-end solution that can easily be added to the final product. As for testing, @Alex-Yarkosky and I are working diligently to finish the test cases required for these. Once we have resolved the aforementioned testing/logical issues (with regard to the fact that we could not have added this onto the website as we had no documentation or information on that portion of the project), would this PR be suitable for merging? |
@shafferz @Alex-Yarkosky there should at the very minimum be a way to access this feature on the command line, which would be straightforward to implement. Take a look at |
Updated corresponding docstrings as well
I have updated this PR to have both written functions be tested. After reviewing the
As such, I believe there is no reason for this PR not to be merged. Accessing the functions we have written through the command-line would be unwieldy, and, frankly, is a different issue entirely, for which I will take the liberty of raising a separate issue on the GitHub Issue Tracker. Considering that six team members' grades would be negatively (and wrongly) affected if this PR is upheld any longer, and the code is sound and strictly follows the contributing guidelines, I implore any technical lead (@Michionlion, @sutterj, or @toccinAC) or even @gkapfham to merge this PR immediately. |
Requested changes have been addressed.
Description of Pull Request
This PR adds a way for the random grouping algorithm to minimize conflicts, including assuring that the worst possible groupings are avoided due to running through multiple iterations.
Fixes #107
Type of Change
Please describe the pull request as one of the following:
Tags
Reviewers: @gkapfham @Michionlion @aubreypc @huangs1 @Lancasterwu @quigley-c @barrezuetai @sutterj @toccinAC
Assignees: @Alex-Yarkosky @shafferz @ohnoanarrow @livingstonp729 @Spurrm @robert-samuel07