Skip to content

Conversation

@Kh4ster
Copy link
Contributor

@Kh4ster Kh4ster commented May 20, 2025

This PR fixes two issues:

  1. Solution file generated by PDLP presented several JSON issues, was still mentioning ms for the solve time and was not handling the potential missing fields from Dual Simplex. To handle the last and so that user can know, a bool was added to know which solver solved the LP problem
  2. The default value for binary time limit was an int resulting in a bad any cast in case it was not set

@Kh4ster Kh4ster requested review from a team as code owners May 20, 2025 08:20
@Kh4ster Kh4ster requested review from Iroy30, akifcorduk and kaatish May 20, 2025 08:20
@Kh4ster Kh4ster added bug Something isn't working non-breaking Introduces a non-breaking change pdlp labels May 20, 2025
Copy link
Contributor

@aliceb-nv aliceb-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :) Thanks!

double solve_time;

/** Whether the problem was solved by PDLP or Dual Simplex */
bool solved_by_pdlp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tangential remark but should the members of additional_termination_information_t possess default initializers? This is essentially a plain struct and I see that its instanciation sites initialize members manually, this might lead in the future to missed initializations/garbage output in write_additional_termination_statistics_to_file (e.g. if in some future codepath we forget to set solved_by_pdlp)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think I agree. Should we initialized it with a "zero" value or "nan" value so that if those value appear we know we forgot something?

Copy link
Contributor

@aliceb-nv aliceb-nv May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote would be for NaN :) 0 may be valid in some context / mislead whoever is reading the solution log into thinking this is expected output

@rg20 rg20 added this to the 25.05 milestone May 20, 2025
Note: Applicable to only MILP
Time used for pre-solve
solve_time: Float64
Solve time in milliseconds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be seconds now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes good catch

@copy-pr-bot
Copy link

copy-pr-bot bot commented May 22, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Iroy30
Copy link
Member

Iroy30 commented May 22, 2025

/ok to test

@copy-pr-bot
Copy link

copy-pr-bot bot commented May 22, 2025

/ok to test

@Iroy30, there was an error processing your request: E1

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/

@Iroy30
Copy link
Member

Iroy30 commented May 22, 2025

/ok to test 8953319

@Iroy30 Iroy30 self-requested a review May 22, 2025 03:02
@Iroy30
Copy link
Member

Iroy30 commented May 22, 2025

/merge

@rapids-bot rapids-bot bot merged commit 6975e1c into NVIDIA:branch-25.05 May 22, 2025
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change pdlp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants