mggg / VoteKit

A Swiss Army Knife for computational social choice research
https://votekit.readthedocs.io/en/latest/
MIT License
10 stars 12 forks source link

add new arguments #96

Closed cdonnay closed 11 months ago

cdonnay commented 11 months ago

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

jamesturk commented 11 months ago

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.

cdonnay commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

Awesome, sounds good to me.