pbvoting / pabutools

A complete set of tools to work with participatory budgeting elections.
https://pbvoting.github.io/pabutools/
GNU General Public License v3.0
7 stars 12 forks source link

Add priceability module #26

Closed drknzz closed 4 months ago

drknzz commented 4 months ago
Simon-Rey commented 4 months ago

Dear Kamil,

Thank you so much for the PR. It is much appreciated. Just two comments:

I did not go through the code in details but I trust you that it works as expected.

Simon.

Simon-Rey commented 4 months ago

By the way, the docs thing does not have to be now, but it's good to do it at some points. You can wait to have a stable version of your function to do so.

drknzz commented 4 months ago

Removed the deprecated typing imports 👍

As for the tests, it seems that there have been some precision problem when running the model with the cbc solver. Locally I am using gurobi instead, so that's why I haven't caught it previously. I've tweaked some parameters in mip and it seems to be working with the cbc solver.

drknzz commented 4 months ago
  • It would be nice to include how to use your code in the docs. That should go to the file pabutools/docs-source/source/usage/analysis.rst. I made you a priceability section there. You can take example from other parts of the docs to see what to put.

Sure thing, there should be some guide on the function. Will submit a separate PR, after the relaxation part, as suggested.

Simon-Rey commented 4 months ago

Great, that's all fixed now.

Another thing I'm only seeing now: for both validate_price_system and priceable you have a * argument. What's the motivation there? I do think them being used and in general I think it's cleaner to restrict to the arguments you need.

drknzz commented 4 months ago

Great, that's all fixed now.

Another thing I'm only seeing now: for both validate_price_system and priceable you have a * argument. What's the motivation there? I do think them being used and in general I think it's cleaner to restrict to the arguments you need.

I don't think I quite follow. The *, makes it so the following arguments are keyword-only, so one has to be explicit when passing them. They can be, and are meant, to be used as usual. You cannot pass more arguments than the specified positional arguments when using * (as they are not bound to a variable, like in *args).

i.e. calling

fun(1, 2, 3, c=4)
fun(1, 2, 3)

for a function signature

def fun(a, b, *, c=10)

wouldn't work

Simon-Rey commented 4 months ago

Ooh I see, I did not know that. Thanks for the tip :)