Closed LilithHafner closed 10 months ago
Merging #519 (bfd023d) into master (5d0d7e0) will decrease coverage by
0.63%
. The diff coverage is83.33%
.
@@ Coverage Diff @@
## master #519 +/- ##
==========================================
- Coverage 54.25% 53.63% -0.63%
==========================================
Files 51 52 +1
Lines 3854 3897 +43
==========================================
- Hits 2091 2090 -1
- Misses 1763 1807 +44
Files | Coverage Δ | |
---|---|---|
ext/PythonCallExt.jl | 100.00% <100.00%> (ø) |
|
src/problems/analytical_problems.jl | 100.00% <100.00%> (ø) |
|
src/problems/bvp_problems.jl | 33.33% <100.00%> (+1.90%) |
:arrow_up: |
src/problems/dae_problems.jl | 100.00% <100.00%> (ø) |
|
src/problems/dde_problems.jl | 26.19% <100.00%> (+1.80%) |
:arrow_up: |
src/problems/discrete_problems.jl | 72.00% <100.00%> (+1.16%) |
:arrow_up: |
src/problems/rode_problems.jl | 100.00% <100.00%> (ø) |
|
src/problems/sdde_problems.jl | 69.23% <100.00%> (+2.56%) |
:arrow_up: |
src/problems/sde_problems.jl | 58.97% <100.00%> (+1.07%) |
:arrow_up: |
src/problems/steady_state_problems.jl | 66.66% <100.00%> (-15.16%) |
:arrow_down: |
... and 4 more |
... and 5 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
I think this makes sense. The other place that it could be would be the DiffEqBase solve.jl, which intercepts right before solve and makes conversions so that the problem is solvable by a given solver (and does a bunch of error throws according to the options). However, that is done at a later time because those conversions can be dependent on the solver that is chosen. Here, this would be conversions that should just always happen. So I guess this is needed here, though it is a bit tedious to add it everywhere
Force push was a clean rebase onto master
However, that is done at a later time because those conversions can be dependent on the solver that is chosen
I agree that delaying these conversions is, unfortunately, probably not a great idea. For example, I think it would be reasonable when choosing a solver to perform a query on u0
or f
that would fail if we hadn't converted from Python yet (e.g. any(isnan, u0)
fails on Python floats)
Also, looking at DiffEqBase/src/solve.jl, it seems that it would still be a bit messy to extract u0 and all user functions and convert them.
I'll proceed with adding these conversions to all entrypoints I can find.
Makes sense
CodeCov claims this has pretty high patch coverage, but that is sort of a lie. In theory, this PR enables full usage of all of DifferentialEquations via PythonCall. To test that claim would require rewriting all downstream tests in Python. That's probably not worth doing, but I want to be clear that if I failed to insert a call to convert_initial_state
somewhere, or someone else removes some of those calls later, that will not be caught by CI. I can add more tests if you think they are necessary.
@avik-pal @ErikQQY there's still one last remake issue with BVPs: https://github.com/SciML/SciMLBase.jl/actions/runs/6439762476/job/17487819266?pr=519#step:6:880
The failing tests are about TwoPointBVPFunction
remake, we still need a dispatch for remake
, maybe continue #517?
That PR is stale though since the function form was updated and the bc
parts were already removed from the problem, but yes something like that PR but for the updated function form is required.
The goal of this PR is to make PythonCall and the DifferentialEquations ecosystem fully compatible, making https://github.com/SciML/diffeqpy/pull/118 trivial.
I think I've implemented the nontrivial design decisions that have to be made, so this is ready for review. If the design looks good and once #502 merges, I'll finish the details to get this to a mergeable state.