Type examples/ #819

frrad · 2025-03-28T06:23:09Z

Adds typing to files in examples/.

Also removed several more pulp/ files from the mypy blacklist, adding type: ignores where necessary.

examples/furniture.py

pchtsp

Thanks! I added minor questions about the renaming of some objects. I'm curious what tool renames objects based on their implied type. It sounds a bit weird.

pchtsp · 2025-03-28T14:34:35Z

examples/SpongeRollProblem4.py

    lenOpts = [5, 7, 9]

-    def __init__(self, name, lengths=None):
+    def __init__(self, name: str, lengths: List[int]) -> None:


shouldn't we keep the =None here?

I removed it because the zip below on L87 fails to typecheck if lengths is None. Since this code is presumably working I took that to mean that this is never used that way in practice.

pchtsp · 2025-03-28T14:35:34Z

examples/CGcolumnwise.py

    lenOpts = ["5", "7", "9"]
    numPatterns = 0

-    def __init__(self, name, lengths=None):


also here, should we keep the =None?

pchtsp · 2025-03-28T14:35:56Z

examples/BeerDistributionProblem_resolve.py

 # The problem data is written to an .lp file
 prob.writeLP("BeerDistributionProblem.lp")
-for demand in range(500, 601, 10):
+for demand_value in range(500, 601, 10):


same as with _dict, is this some kind of naming convention based on the type of the object?

Similar to previously there is already a demand in scope and this was me fixing the collision.

christiansegercrantz · 2025-03-28T14:46:49Z

Thanks! I added minor questions about the renaming of some objects. I'm curious what tool renames objects based on their implied type. It sounds a bit weird.

This naming convention is in my opinion a bit outdated with modern typing. Instead of changing names to indicate type it would be better, as is being done in the linked PR, to indicate the type properly with typing.

MBradbury · 2025-03-28T14:48:51Z

Few comments based on #807:

Why use List and not list now that 3.8 has been dropped?
I tried to check the type hints from the previous PR and different tools work differently. For example, I use pyright which is typically significant stricter than mypy and pyright still identifies some issues.
In Type hints #807, I also did a lot of typing work in examples/, its not likely that I will get round to fixing that PR, so might be worth taking a look and extracting what is needed.

MBradbury · 2025-03-28T14:52:29Z

pulp/pulp.py

 try:  # allow Python 2/3 compatibility
    maketrans = str.maketrans
 except AttributeError:
-    from string import maketrans


Why not just change to be: from string import maketrans, Python 2 is not supported

MBradbury · 2025-03-28T14:54:10Z

pulp/pulp.py


 _DICT_TYPE = dict

 if sys.platform not in ["cli"]:


There is no sys.platform which is cli any more. I presume this was IronPython. Why not simplify and just remove _DICT_TYPE as per https://github.com/coin-or/pulp/pull/807/files#diff-b2a258edfe0fa294f032a978656a4082842558faa98b0950d9f68c50f546bbe2L151

MBradbury · 2025-03-28T14:56:20Z

pulp/pulp.py

        """
        self.expr = e if isinstance(e, LpAffineExpression) else LpAffineExpression(e)
        self.name = name
-        self.constant: float = self.expr.constant


This just hides a potential bug. Better to fix the annotations

https://github.com/coin-or/pulp/pull/807/files#diff-b2a258edfe0fa294f032a978656a4082842558faa98b0950d9f68c50f546bbe2R169

https://github.com/coin-or/pulp/pull/807/files#diff-b2a258edfe0fa294f032a978656a4082842558faa98b0950d9f68c50f546bbe2R1212

In my PR, I added some type aliases, which I then used where appropriate.

LptNumber: TypeAlias = int | float LptItem: TypeAlias = Union[LptNumber, "LpVariable"] LptExpr: TypeAlias = Union[LptItem, "LpAffineExpression"] LptConstExpr: TypeAlias = Union[LptNumber, "LpAffineExpression"] LptExprConstraint: TypeAlias = Union[LptExpr, "LpConstraint"] LptVars: TypeAlias = "LpVariable"

MBradbury · 2025-03-28T14:57:02Z

pulp/pulp.py

    def valid(self, eps: float = 0) -> bool:
        val = self.value()
        if self.sense == const.LpConstraintEQ:
-            return abs(val) <= eps


Again, just hides the bug: https://github.com/coin-or/pulp/pull/807/files#diff-b2a258edfe0fa294f032a978656a4082842558faa98b0950d9f68c50f546bbe2R1370

My links don't seem to have worked, there needs to be a check like:

val = self.value() if val is None: return False

MBradbury · 2025-03-28T14:57:45Z

pulp/pulp.py

        if name is not None:
            self.__name = name.translate(LpAffineExpression.trans)
        else:
-            self.__name = None


Not sure why this is needed?

MBradbury · 2025-03-28T15:07:01Z

There are some easy wins in the type hinting space, but as I found in #807, there are some architectural issues with LpElement, LpVariable and LpConstraintVar that need to be fixed. Also FractionElasticSubProblem just seems fundamentally broken.

Then there is also the question of what type the coefficient should be? int, float, decimal, numpy int/float types, or what combination of these?

MBradbury · 2025-03-28T15:26:02Z

examples/SpongeRollProblem5.py

    Patterns += [Pattern("P" + str(len(Patterns)), i)]

 # This loop will be repeated until morePatterns is set to False
 while morePatterns == True:


Remove the == True

I am trying to keep the scope of this PR as limited as possible. There's a ton of less-than-ideal code here. If I started fixing things I don't know where I would be able to stop.

Same problem I had in #807 :)

MBradbury · 2025-03-28T15:27:17Z

examples/SpongeRollProblem5.py

 while morePatterns == True:
    # Solve the problem as a Relaxed LP
    duals = masterSolve(Patterns, rollData)
+    assert isinstance(duals, dict)


Surely, the return type is a tuple as per masterSolve?

that function returns a union type. this assert is necessary to tell mypy which part of the union this is.

Should the return should be duals, _ = masterSolve(Patterns, rollData) then?

frrad · 2025-03-28T15:33:24Z

👋 thanks for all the reviews.

To clear up some possible confusion - this PR is intended to be the minimum possible change to add types to the examples/ directory. It is explicitly not trying to do things like fix types in pulp/ or improve the non-type code in examples. I think structuring things this way will lead to a change which is easy to review and a pareto improvement over the current situation. There will still definitely remain a lot of room to improve things afterwards, but by trying to be focused here I think we can hopefully get this one in without too much back and forth and make the next PR easier for whoever ends up making it.

Additional responses inline:

Thanks! I added minor questions about the renaming of some objects. I'm curious what tool renames objects based on their implied type. It sounds a bit weird.

This naming convention is in my opinion a bit outdated with modern typing. Instead of changing names to indicate type it would be better, as is being done in the linked PR, to indicate the type properly with typing.

Sorry for the confusion caused by my clunky variable names. They are not meant to convey the type, just fix the variable type change issues. These variables all actually typecheck fine without adding explicit hints.

re: changes to pulp

These are all great questions. The intention of this PR is not to fix any type problems in pulp/. I just added whatever type: ignore were necessary to allow me to remove these files from the mypy blacklist. I definitely agree that a lot more work is needed to actually fix the issues there.

MBradbury · 2025-03-28T15:35:55Z

re: changes to pulp

These are all great questions. The intention of this PR is not to fix any type problems in pulp/. I just added whatever type: ignore were necessary to allow me to remove these files from the mypy blacklist. I definitely agree that a lot more work is needed to actually fix the issues there.

Would it be better to not add the # type: ignores then? By adding them potential bugs are covered up. IMO, better to let the tests fail and then fix the bug than to hide the issue.

frrad · 2025-03-28T15:56:21Z

In general unit tests / CI work best if you keep them in a state where you always expect them to pass. You want their binary pass / fail state to be a useful signal of whether or not a new change introduces new bugs. If you add tests which you expect to fail it can confuse things quite a bit.

The type ignore statements are a useful crutch to be able to make incremental improvements without having to fix everything at once. With their help we can do

The current state mypy doesn't even consider these files and CI gives us no information about what changes do to typing in them
After we add ignores readers of the code will at least be aware of existing type issues.
Any future PRs which touch these files will fail CI if they introduce new type errors.
New PRs can be made which fix the existing type issues and remove the ignore comments. Reviewers will be able to be confident in the correctness of these PRs because CI status will tell them whether or not mypy passes

christiansegercrantz · 2025-03-31T06:57:38Z

Thanks! I added minor questions about the renaming of some objects. I'm curious what tool renames objects based on their implied type. It sounds a bit weird.

This naming convention is in my opinion a bit outdated with modern typing. Instead of changing names to indicate type it would be better, as is being done in the linked PR, to indicate the type properly with typing.

Sorry for the confusion caused by my clunky variable names. They are not meant to convey the type, just fix the variable type change issues. These variables all actually typecheck fine without adding explicit hints.

Makes sense, didn't read the code in larger context. Good change then, at some point those should probably have completely different names to convey their meaning better. But a good first step.

frrad · 2025-04-02T03:50:27Z

merged master to pick up #820 - looks like I have some work to do

frrad · 2025-04-02T04:05:37Z

That wasn't too bad at least 😅

@pchtsp I think I have addressed all your comments. Please let me know if there is anything more I can answer or change.

There are some references to these files in the documentation by line number like this:

pulp/doc/source/CaseStudies/a_blending_problem.rst

Lines 97 to 98 in f626d34

    
           .. literalinclude:: ../../../examples/WhiskasModel1.py 
        
               :lines: 7-8

which may need to be updated if things have shifted around. I'm happy to have a look at this as well as part of this PR. I want to wait until the actual code changes look good first though in case the lines end up moving around more.

pchtsp · 2025-04-04T11:44:34Z

thanks @frrad . I don't mind if we do not have a perfect state now. Anything that gets as closer is good.
@christiansegercrantz , @MBradbury thanks for your help, if you can make any improvement on this on a separate PR (e.g., through #807) I can review it and merge it. Again, if we do it in pieces, we will probably finish sooner.

Thanks again

* initial pass * black + isort * add ignores in pulp.py and pulp/__init__.py * start fixing mypy errors * fix some mypy * mypy passes * move Pattern class definition to be before use

* remove build from gitignore * commit doc build output * fix references * remove compile output * Type examples/ (#819) * initial pass * black + isort * add ignores in pulp.py and pulp/__init__.py * start fixing mypy errors * fix some mypy * mypy passes * move Pattern class definition to be before use * restore gitignore * whiskas 1 * whiskas 2

frrad added 6 commits March 26, 2025 23:21

initial pass

650a30b

black + isort

7f0cf0a

add ignores in pulp.py and pulp/__init__.py

e524bfa

start fixing mypy errors

fa54838

fix some mypy

1c5f765

mypy passes

038c58e

pchtsp reviewed Mar 28, 2025

View reviewed changes

examples/furniture.py Show resolved Hide resolved

pchtsp reviewed Mar 28, 2025

View reviewed changes

pchtsp mentioned this pull request Mar 28, 2025

Type hints #807

Draft

MBradbury reviewed Mar 28, 2025

View reviewed changes

Merge remote-tracking branch 'origin/master' into type_examples

8c80641

move Pattern class definition to be before use

2e52383

pchtsp merged commit ba0efaa into coin-or:master Apr 4, 2025
14 checks passed

frrad mentioned this pull request Apr 5, 2025

fix set partition problem case study docs #827

Merged

Uh oh!

Type examples/ #819

Type examples/ #819

Uh oh!

Conversation

frrad commented Mar 28, 2025

Uh oh!

Uh oh!

pchtsp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christiansegercrantz commented Mar 28, 2025

Uh oh!

MBradbury commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MBradbury commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frrad commented Mar 28, 2025

Uh oh!

MBradbury commented Mar 28, 2025

Uh oh!

frrad commented Mar 28, 2025

Uh oh!

christiansegercrantz commented Mar 31, 2025

Uh oh!

frrad commented Apr 2, 2025

Uh oh!

frrad commented Apr 2, 2025

Uh oh!

pchtsp commented Apr 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants