Thanks, Florian!

I did a commit with changes to the three files including Pillow. Surprisingly, depandabot only had marked two of them but I don't think it hurts to update all three. Depandabot then automatically tried to rebase the pull request, figured out that the issue disappeared and closed them.

(Unfortunately in these of causing some extra work.)

It would make sense to fix this for both pull requests together in a single commit.

Yes, unfortunately I think that's what we need to do.

Fine with me. Is it OK to just accept the pull request, though? Its commits do not follow our naming scheme. Should I just do a commit on main instead?

I see two more options: we could disable dependabot for the repo or click the "dismiss" button but in both cases all forks would probably still get the reports. Therefore, I think it's best to just accept the pull requests.

I know. But I think we need to do something -- at minimum, decline the pull requests.

One risk I see with this is that our project will be labelled as being subject to security vulnerabilities, which is then something we have to explain away etc. It may be easier just to apply such fixes. An alternative is to change the way we archive experiments.

This is an automated pull request of dependabot. It changes the version of an imported package from 8.1.0 to 8.1.1, in the requirements.txt file that is used to prepare the virtual environment for the lab experiment of that issue. As this is there mostly for archival purposes, I don't see an immediate need to change it. But I'm also not against it. I'm a bit worried that we'll get lots of those pull requests from dependabot because every lab experiment after a certain date will have such a requirements.txt to archive the environment that the experiment ran in.

We have a pull request related to this issue to fix a security vulnerability in Pillow, used in the issue experiments. Can whoever created the experiment (Florian?) have a look at the pull request?

Thanks everyone. I added a description for the changelog and will now merge.

For future reference, here are the results of an experiment with the new code. It ran operator-counting LP and MIP heuristics with the state equation and LM-cut constraints. There are 3 unexplained errors in satellite and organic synthesis, which I think all happen because the tasks are massive. There is also one case where the MIP has a lower value than the LP. This shouldn't normally happen, but the case is in trucks-strips, where the preprocessing is non-deterministic, so I think all of this is fine.

In general, computing the MIP is about one order of magnitude more expensive than the LP and the difference in initial heuristic value is modest in this configuration.

https://ai.dmi.unibas.ch/_tmp_files/pommeren/issue891-v1-mips-issue891-v1-issue891-v1-total_time-opcount-lp-opcount-mip.png
https://ai.dmi.unibas.ch/_tmp_files/pommeren/issue891-scatterplot.png

https://ai.dmi.unibas.ch/_tmp_files/pommeren/issue891-lp-mip-compare.html

I had a look at which parts of the code it touches and at the (low) code complexity, and based on this I don't need to be involved in the review.

Fine by me, the code looks good.

I think so as well. Are you fine with a review by Jendrik, or should we hold until you get a chance to look? Jendrik, are you fine with merging?

From your description, it does not sound that the experiments would be a reason not to merge.

I ran experiments and saw the same effect with memory usage of SoPlex when using potential heuristics, we saw in issue960. The time distribution was also similar to that of issue960: most tasks within a factor of 1.05 and a couple of tasks with a factor of 1.8. Again, there were not many tasks with factors in between those and there is no obvious trend in one direction or another. I uploaded an example here (the other configurations look similar):
https://ai.dmi.unibas.ch/_tmp_files/pommeren/issue891-v1-issue891-base-issue891-v1-total_time-diverse-potentials-soplex.png


The full report shows that we lost some problems but looking at the scatter plots, it looks like this are the ones that used time close to the limit before and got pushed over the edge by the random noise.

https://ai.dmi.unibas.ch/_tmp_files/pommeren/issue891-v1-issue891-base-issue891-v1-compare.html

Should we merge this anyway?

This was lying around for a while. I updated the code and the pull request:
https://github.com/aibasel/downward/pull/13

New experiments are running.

Thanks. It will take a while through their pipeline until it is part of a release and then a while longer until we test their new release and change our recommended version in our installation instructions. So for now everyone running into this problem should still use the workaround of adding a trivial constraint in case the loaded LP is empty.

The OSI issue referenced at msg9210 has been resolved and closed today.

The issue is reported to the OSI developers and can be found at https://github.com/coin-or/Osi/issues/129.

Thanks for reporting it here. We already discussed this by email but for the record: I think this is an issue with OSI. Whether or not it would be counted as a bug there depends on whether they view setting integer constraints on an empty LP as a valid thing to do. I think it would still be worth reporting to their tracker to see if they want to change this. Clemens, if you open an issue, please link it here, so we see if the bug gets fixed.

On our side, we could check for this case when loading a problem and exit the planner with a more obvious error message. Until this is fixed, adding a trivial constraint (like "-infty <= + infty" or "0 <= 0") should prevent the error in these cases.

When using this feature for an experiment I encountered the following error:

OsiCpxSolverInterface.cpp:2142: virtual void
OsiCpxSolverInterface::setInteger(int): Assertion `coltype_ != NULL' failed.

It occurs when calling the load_problem(...) in the LPSolver class with an 
empty constraints vector. Maybe this happens, because LP variables are only 
initialized once they are found in a constraint. Therefore setting the type to 
integer before this happens fails.

Hi all, there was activity on this issue while the mail server moved. If you
don't want to miss progress on this issue, please check in the tracker if there
was anything you missed.

Upvote for integer variables :)

Me too. Is there consensus that we want to add this feature?

If yes, we should also do an experiment. I would not want to have code in the
planner that we have never tried out experimentally.

I left some comments on Bitbucket.

I added a pull request for this on bitbucket:

https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/55

Moving Michael's comment from the summary to a change note:

> Linear programming support in Fast Downward does not allow to specify that some 
> variables are binary/integer, disallowing solving ILP.