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

Type hints #807

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Type hints #807

wants to merge 11 commits into from

Conversation

MBradbury
Copy link
Contributor

I have seen #755 and wanted to take a look at providing type hints also. I would appreciate a review of this, as there are some choices I made which need to be checked and others where a decision needs to be made.

Summary:

  • Increases minimum version to Python 3.9, as this is when hinting list[T] was introduced (instead of using List[T])
  • Added Lpt* type aliases to avoid excessive Unions across the code
  • Overloads for dict, dicts and matrix
  • Most of the examples type check successfully
  • Removed LpElement
  • Needed to move away from dicts to dataclasses to serialise MPS. Ability to convert to dicts is provided by Python, but a separate package (dacite) has been needed to load from json into the dataclasses.
  • Removed the ability for `LpAffineExpression to inherit from an OrderedDict, always inherit from dict.

Choices to make:

  1. What type should be used to store coefficients? Currently a mixture of int and float is allowed.
  2. dicts and matrix cannot be fully typed with Python's type hints. I have added overloads for 3 dimensional inputs, how many is appropriate?
  3. What is LpConstraintVar? Is it a variable or is it a constraint? There are several pieces of code in LpAffineExpression that assume that all the items it contains are either a LpVariable or a LpConstraintVar. Several options:
    • Could LpConstraintVar inherit from LpVariable?
    • Manage dict of LpConstraintVar separately?
    • Does LpConstraintVar support operators in a LpAffineExpression?
  4. There seems to be a number of problems with FractionElasticSubProblem as parameters seemed to be in the incorrect order for the __init__.

There is also a test failure to investigate (PULP_CBC_CMDTest.test_dual_variables_reduced_costs), but I am having trouble finding the root cause.

@MBradbury MBradbury marked this pull request as draft February 26, 2025 08:06
@pchtsp
Copy link
Collaborator

pchtsp commented Feb 26, 2025

Thanks, I will happy contribute to this initiative of adding types.

I have no problem moving to 3.9, although if it's only to avoid writing from typing import List then it's a bit too much, isn't it?
Moving to dataclasses seems a good idea. I'm not well versed on them so I cannot say much. I know pydantic is also used to type objects, not sure if relevant (I don't regularly used these libraries).
On the dacite question, I would try hard not to add new dependencies unless really justified. The package currently has none and I think that's quite nice.

Many of your questions are for @stumitchell to answer, I believe.
On the first two questions:

  • What type should be used to store coefficients? Currently a mixture of int and float is allowed.
    • Without checking the "int" cases I would assume coefficients should be float (because that's what LP understands best).
  • dicts and matrix cannot be fully typed with Python's type hints. I have added overloads for 3 dimensional inputs, how many is appropriate?
    • It's hard to impose anything... I myself make dicts of tuples or various size. But part of the charm of PuLP is that it's very flexible and people use it very differently

@christiansegercrantz
Copy link
Contributor

Amazing work, thank you! I can also help with reviewing and changes if needed, although currently a bit busy.

Regarding python 3.8, it's already end-of-lifecycle, so I don't see a reason not to move away from it, people shouldn't be using it anyway.

@MBradbury
Copy link
Contributor Author

For dacite, alternatively, pulp moves to pickle for disk serialisation which would eliminate the need for dacite, but then human-readability of the files would be lost.

The type hints do not prevent dicts / matrix being used with more dimensions, just means that IDEs won't provide accurate information on the returned type.

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.

3 participants