search

GatorIncubator / gatorgrouper

:busts_in_silhouette: Automated Group Formation Tool Enabling Effective Team Work
GNU General Public License v3.0
20 stars 20 forks source link

Refactor/remove group size #260

Closed paladp closed 5 years ago

paladp commented 5 years ago

Description of Pull Request

This pull request changes the group_creation, gatorgrouper_cli, and test files primarily in order to remove the group_size user option. Changes to group_creation remove the corresponding grouping methods and leave the names of the num_group methods unchanged. The changes to the gatorgrouper_cli serve to change the logic for selecting a grouping method to reflect the removal of the previous methods. Lastly, two test files were changed since they called the now deleted functions. Where possible, I tried to change the tests to the best of my ability to be able to work with the new group_creation methods. Where I could not change them or did not know whether I should, I commented them out and left a note in the comments. I'm not entirely confident in the test changes so help from @Lancasterwu and the rest of the testing team would be appreciated.

Fixes #253 Fixes #266

Type of Change

Please describe the pull request as one of the following:

Tags

@aubreypc @Michionlion @Lancasterwu @huangs1 @Alex-Yarkosky

Lancasterwu commented 5 years ago

@Michionlion It does since graph algorithm only use and must use --num-group,

Michionlion commented 5 years ago

@Michionlion It does since graph algorithm only use and must use --num-group,

From what I could see there is no path in gatorgrouper_cli.py that checks the method for graph.

Lancasterwu commented 5 years ago

@Michionlion It was there already, we didn't refactor it since it only uses numgroup.

elif (
            GG_ARGUMENTS.method == constants.ALGORITHM_GRAPH
            and GG_ARGUMENTS.num_group is not constants.DEFAULT_NUMGRP
        ):
            GROUPED_STUDENT_IDENTIFIERS = group_graph.group_graph_partition(
                SHUFFLED_STUDENT_IDENTIFIERS, GG_ARGUMENTS.num_group
            )

However, there was a bug that if we don't use --num-group with --graph and only use --graph the program will run group_random. We can fix that bug with a different PR or just in PR#261.

paladp commented 5 years ago

@Michionlion It was there already, we didn't refactor it since it only uses numgroup.

elif (
            GG_ARGUMENTS.method == constants.ALGORITHM_GRAPH
            and GG_ARGUMENTS.num_group is not constants.DEFAULT_NUMGRP
        ):
            GROUPED_STUDENT_IDENTIFIERS = group_graph.group_graph_partition(
                SHUFFLED_STUDENT_IDENTIFIERS, GG_ARGUMENTS.num_group
            )

However, there was a bug that if we don't use --num-group with --graph and only use --graph the program will run group_random. We can fix that bug with a different PR or just in PR#261.

This has been fixed in the latest commit. Now, when I run 

pipenv run python3 gatorgrouper_cli.py --file students.csv --method graph

I get 

usage: gatorgrouper_cli.py [-h] [-d] [-v] --num-group NUM_GROUP --file FILE
                           [--method {graph,rrobin,random}]
                           [--absentees ABSENTEES [ABSENTEES ...]]
gatorgrouper_cli.py: error: the following arguments are required: --num-group

And running

pipenv run python3 gatorgrouper_cli.py --file students.csv --method graph --num-group 4

Gives us

GatorGrouper: Automatically Assign Students to Groups
https://github.com/GatorEducator/gatorgrouper

Group 1
student6
student1
student7

Group 2
student3
student12
student11

Group 3
student2
student8
student4

Group 4
student10
student5
student9

As expected