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 new arguments #96

Merged
merged 5 commits into from
Sep 21, 2023
Merged

add new arguments #96

merged 5 commits into from
Sep 21, 2023

Conversation

cdonnay
Copy link
Collaborator

@cdonnay cdonnay commented Sep 7, 2023

Add a rank_cols argument to load_csv so that the user can specify which columns of the csv correspond to rankings. Helpful, for example, with the 2013 Minneapolis data file on MGGG/chicago which has extraneous columns.

Also removed quota parameter from Election.get_threshold method since quota should be an attribute of the Election.

Also, this is my first pull request; Moon has asked me to tag y'all going forward. Let me know if you don't wish to be tagged or if I made an error in my request. Thanks!
@jamesturk @drdeford @jgibson517 @ziglaser @jennjwang

Add a rank_cols argument to load_csv so that the user can specify which columns of the csv correspond to rankings. Helpful, for example, with the 2013 Minneapolis data file on MGGG/chicago which has extraneous columns.

Also removed quota parameter from Election.get_threshold method since quota should be an attribute of the Election.
@jamesturk
Copy link
Collaborator

Hi @cdonnay,

A couple of notes:

  1. You'll want to add .idea to .gitignore, and remove the .idea files, those are only relevant to you/PyCharm and shouldn't be part of this repo.
  2. You're adding a required parameter, but it should probably be optional since it was already working without that. This is also why the tests are failing. (A new test should probably be added to accept this optional parameter.)

I'll let someone else comment on the self.quota issue as I haven't kept up with the API in that level of detail.

Thanks, @jamesturk ! I added .idea to .gitignore, and gave the `rank_cols` a default argument.

I think it's worth discussing whether or not you actually want the `rank_cols` parameter to have a default argument. My argument for not including a default is that we don't want to force preprocessing of the csv file onto the user, and we want the user to think intentionally about loading in their data. Happy to hear other ideas about why we might want a default.
Default needs to be a list since that is the declared data type.
Fixing a line that was too long for test.
Use .lower() method to allow users to input quotas.
@cdonnay
Copy link
Collaborator Author

cdonnay commented Sep 8, 2023

Thanks, @jamesturk ! I added .idea to .gitignore, and gave the rank_cols a default argument.

I think it's worth discussing whether or not you actually want the rank_cols parameter to have a default argument. My argument for not including a default is that we don't want to force preprocessing of the csv file onto the user, and we want the user to think intentionally about loading in their data. Happy to hear other ideas about why we might want a default.

@jennjwang
Copy link
Collaborator

Hi @cdonnay thanks for helping with Votekit! I agree with James on making rank_cols an optional argument - I think the user should have an idea of what their data looks like before loading it, but it would be annoying for them to specify the rank_cols every time if their csv file is already clean without extraneous columns. A default would save them this trouble.

@cdonnay
Copy link
Collaborator Author

cdonnay commented Sep 13, 2023

Awesome, sounds good to me.

@cdonnay cdonnay mentioned this pull request Sep 13, 2023
@@ -61,20 +61,18 @@ def __init__(
self.transfer = transfer
self.seats = seats
self.tiebreak = tiebreak
self.threshold = self.get_threshold(quota)
self.quota = quota.lower()
Copy link
Member

Choose a reason for hiding this comment

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

👍 agree with this change

@@ -55,6 +59,12 @@ def load_csv(
if id_col is not None and not df.iloc[:, id_col].is_unique:
raise DataError(f"Duplicate value(s) in column at index {id_col}")

if rank_cols:
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -5,4 +5,4 @@ htmlcov/
.DS_Store
dist/
.ipynb_checkpoints

.idea
Copy link
Member

Choose a reason for hiding this comment

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

you may need to delete the .idea and inspectionProfiles files from the version of your repo on github, but then the .gitignore should handle them in the future!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I think I just deleted those files.

@jgibson517 jgibson517 merged commit 2cd0800 into mggg:main Sep 21, 2023
@cdonnay cdonnay deleted the read_csv branch October 4, 2023 12:57
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.

4 participants