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 priceability module #26

Merged
merged 4 commits into from
Apr 26, 2024
Merged

Conversation

drknzz
Copy link
Contributor

@drknzz drknzz commented Apr 25, 2024

@Simon-Rey
Copy link
Member

Dear Kamil,

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

  • The tests fail for Python 3.10 and 3.9, check what the problem is.
  • I do not want to use the "typing" module which may be deprecated in the future (see e.g., https://stackoverflow.com/questions/66738753/python-typing-deprecation). You can simply use the built-in types for typing list and dict.
  • 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.

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

Simon.

@Simon-Rey
Copy link
Member

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
Copy link
Contributor Author

drknzz commented Apr 25, 2024

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
Copy link
Contributor Author

drknzz commented Apr 26, 2024

  • 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
Copy link
Member

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
Copy link
Contributor Author

drknzz commented Apr 26, 2024

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
Copy link
Member

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

@Simon-Rey Simon-Rey merged commit ce555cb into COMSOC-Community:main Apr 26, 2024
3 checks passed
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.

2 participants