PB- EAR implementation #43
Conversation
remove unnecessary files replace tabulate with internal formatter
There was a problem hiding this comment.
This file should not be in the PR. You should delete it and then ignore it with .gitigonre
There was a problem hiding this comment.
I do not understand why you changed this file. It should not be part of the PR.
tests/test_pb_ear.py
Outdated
| @@ -0,0 +1,143 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
It seems the pytest import creates an error in the automatic tests. Do you really need this import?
There was a problem hiding this comment.
What is the purpose of this change?
pabutools/rules/pb_ear.py
Outdated
| from pabutools.utils_formatting import format_table | ||
| from datetime import datetime | ||
| import os | ||
| from pabutools.logging_config import logger |
There was a problem hiding this comment.
I would put the single line defining the logger to this file, and then remove the import
|
The test should be implemented with unittest2. Take inspiration from the other tests in the package. |
|
Please don't include the logging mechanism. This has not been implemented at the package level, it should be unified. So far, some rules have a "verbose" argument (see MES) that turn on/off print statements. That should be used for logging purposes (or, update the whole package to be using the logging package). |
|
For typing, do not use from typing import List, Tuple, Set but use directly the builtin types: list, tuple, set. Using the typing package is now old-fashioned and should be avoided. |
|
The implementation of the rule does not follow the standard of the package. A rule should take an instance and a profile as argument, you need to check that the profile is of the correct type and compute the results. In general, try to mimic other implementations to understand how the package work. Right now, your code is more of a standalone thing than a piece of code integrated in the package. |
pabutools/rules/pb_ear.py
Outdated
| f"c_cost={added_cost:.2f}, rel_budget={rel_budget:.2f})" | ||
| ) | ||
|
|
||
| if __name__ == "__main__": |
pyproject.toml
Outdated
| [project.urls] | ||
| "Homepage" = "https://github.com/comsoc-community/pabutools" | ||
| "Bug Tracker" = "https://github.com/comsoc-community/pabutools/issues" | ||
|
|
There was a problem hiding this comment.
Remove the pytest part, we are working with unittest2
|
Please remove all the DS_Store that have been added. |
What does it involve -- only changing the functions using the "verbose" argument to use a logger? Or something else? |
|
One advantage of the logging system is that it allows to easily use the logs in other contexts than printing, e.g. in a demo website such as this one. |
|
I fully agree that using logging is much beter. I just want unity within the package. If we use logging then we use it for the whole package and we offer the options to adjust the setting for the whole package and not just for this rule. I don't think there is much work to change the package to logging. As far as I can tell the parts to update are: priceability, cstv, mes_rule, visualisation. |
|
@vivianu2001 please enable the github actions on your fork so that you can test on your side before sending to the PR. For that, go to the settings of the repo, and then under "Actions" |
|
@Simon-Rey Before I proceed, when would you like logs to be active? |
|
Ok if you want to implement logging for the whole package, please follow this: In particular, there should be no configuration, that should be left to the user. So without a module, create a logger like this: This ensures loggers are hierarchical and namespaced which gives users the freedom to do whatever you want. |
|
Additionally, with regards to the PB-EAR. Couple of comments:
|
pabutools/rules/pb_ear.py
Outdated
| all_projects = set(project_cost) | ||
|
|
||
| # Convert the profile to a list of (weight, preference list) pairs | ||
| voters = [(profile[ballot], list(ballot)) for ballot in profile] |
There was a problem hiding this comment.
I don't understand how this can work if you don't have a MultiProfile. Use profile.multiplicity(ballot) instead.
pabutools/rules/pb_ear.py
Outdated
| # Identify projects whose support justifies their cost (IPSC condition) | ||
| C_star = { | ||
| c for c in available_projects | ||
| if round(candidate_support[c], 6) >= round((initial_n * project_cost[c]) / budget, 6) |
There was a problem hiding this comment.
Also, for the round do not use a fixed value but a optional argument for the function such as rounding_precision = 6. That allows users to change this.
pabutools/rules/pb_ear.py
Outdated
|
|
||
| if N_prime: | ||
| sum_supporters = sum(voter_weights[i] for i in N_prime) | ||
| weight_fraction = total_weight_to_reduce / sum_supporters if sum_supporters > 0 else 0 |
pabutools/rules/pb_ear.py
Outdated
| # Construct the final BudgetAllocation object | ||
| allocation = BudgetAllocation() | ||
| for name in sorted(selected_projects): | ||
| allocation.append(instance.get_project(name)) |
There was a problem hiding this comment.
Why do you not store the Project instance in selected_projects? Rather than having to call get_project for all of them (which iterates through all the projects each time).
|
I just saw your last commit. A lot has changed since we started discussing, don't forget to always synchronise yourself with the latest state of the repository. |
Hey! Thanks for the heads up. I’m still working on it and planning to run all the tests and sync everything properly once I finish. I’ll make sure to update you when it’s ready. |
|
@Simon-Rey Hi, |
|
That looks perfect, thanks for the work you put in here :) |
3 participants
Implementation of the PB-EAR algorithm as described in:
"Proportionally Representative Participatory Budgeting with Ordinal Preferences"
Haris Aziz & Barton E. Lee, 2020
https://arxiv.org/abs/1911.00864v2
Summary of contributions: