Skip to content

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Mar 21, 2025

Preliminary PR to run tests with the new version of NLSolversBase (which includes DI)

How should we document and test this more properly?

@blegat
Copy link
Contributor

blegat commented Mar 22, 2025

The test failures might be fixed by #1073

@gdalle
Copy link
Contributor Author

gdalle commented Mar 22, 2025

Yeah I don't think they're related to this PR

@pkofod pkofod closed this Mar 22, 2025
@pkofod pkofod reopened this Mar 22, 2025
@pkofod
Copy link
Member

pkofod commented Mar 22, 2025

I merged 1073

@pkofod
Copy link
Member

pkofod commented Mar 22, 2025

The test failures might be fixed by #1073

I did merge them, but I'm wondering if those fixes are related to the No nonlinear objective. errors. Could it be that the NDifferentiable are not populating all the fields they should @gdalle ?

@pkofod
Copy link
Member

pkofod commented Mar 22, 2025

or, @blegat looking at

 exclude = String[
            # No objective
            "test_attribute_SolveTimeSec",
            "test_attribute_RawStatusString",

were more tests added that we need to exclude?

@blegat
Copy link
Contributor

blegat commented Mar 23, 2025

Yes, there might just be new tests that need to be excluded

@pkofod
Copy link
Member

pkofod commented Mar 23, 2025

Yes, there might just be new tests that need to be excluded

one of them is this https://github.com/jump-dev/MathOptInterface.jl/blob/6bfca94b728e48a7800caf26bcc749fae86e0bc7/src/Test/test_nonlinear.jl#L1847-L1870 does that need to be excluded?

@gdalle
Copy link
Contributor Author

gdalle commented Mar 23, 2025

Could it be that the NDifferentiable are not populating all the fields they should @gdalle ?

Looking at the NLSolversBase PR again, there is this comment that never got an answer about con_jac! being ignored, but I don't think it affects the objective.
Otherwise I can't see how we would fail to initialize some fields without errors popping up earlier in the test suite.

@gdalle
Copy link
Contributor Author

gdalle commented Mar 23, 2025

I inserted #1133 into this PR

@pkofod
Copy link
Member

pkofod commented Mar 23, 2025

Seems like one is saying that a nonlinear objective is not present but I cannot make out if it should be there or not. The other one is an IPNewton that fails and I'm not sure if that could be related to the constraint function you're referring to above.

@gdalle
Copy link
Contributor Author

gdalle commented Mar 23, 2025

Do those checks also fail on main or #1133 without the new NLSolversBase, eg if we upper bound its compat to before the DI integration?

@pkofod
Copy link
Member

pkofod commented Mar 23, 2025

Ill check later, good question !

@gdalle
Copy link
Contributor Author

gdalle commented Mar 23, 2025

Having a baseline of failing tests is not great for adding new features 😅 if they have been failing for a while perhaps they can be marked as @test_broken

@pkofod
Copy link
Member

pkofod commented Mar 23, 2025

Let's see what we get from #1133

@pkofod
Copy link
Member

pkofod commented Mar 23, 2025

Also failed there. Let's try #1134

@gdalle
Copy link
Contributor Author

gdalle commented Mar 23, 2025

Note that #1133 failed on Julia 1.6 first, where the DI-upgraded NLSolversBase cannot even be installed

@pkofod
Copy link
Member

pkofod commented Mar 23, 2025

You can try to resolve the conflicts due to the bounds I added on master to see what happens here now

@gdalle
Copy link
Contributor Author

gdalle commented Mar 23, 2025

@pkofod I merged master into this branch, let's see what blows up

Copy link

codecov bot commented Mar 23, 2025

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.23%. Comparing base (8e305a4) to head (f49d304).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/multivariate/optimize/interface.jl 72.22% 5 Missing ⚠️
...tivariate/solvers/constrained/ipnewton/ipnewton.jl 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1131      +/-   ##
==========================================
- Coverage   86.57%   86.23%   -0.34%     
==========================================
  Files          45       46       +1     
  Lines        3537     3531       -6     
==========================================
- Hits         3062     3045      -17     
- Misses        475      486      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pkofod
Copy link
Member

pkofod commented Mar 23, 2025

Okay, seems to pass. codecov complains about an unhit method, but it seems to have been like that before as well.

@pkofod pkofod merged commit 298a9a9 into JuliaNLSolvers:master Mar 23, 2025
10 of 12 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.

3 participants