SciML / SciMLBase.jl

The Base interface of the SciML ecosystem
https://docs.sciml.ai/SciMLBase/stable
MIT License
118 stars 91 forks source link

refactor: use `getu`/`setu` for all indexing #721

Closed AayushSabharwal closed 5 days ago

AayushSabharwal commented 2 weeks ago

@TorkelE I moved your symbolic indexing tests here, added some more, and replaced symbol_indexing.jl. I've ensured everything in symbol_indexing.jl is still tested somewhere. Tests pass using MTK master

Checklist

Additional context

Add any other context about the problem here.

AayushSabharwal commented 2 weeks ago

Eventually I'll move these to SII's downstream testset

oscardssmith commented 2 weeks ago

this looks great! one question is now that all these getindexes just call getu, could they all be merged into a single method?

AayushSabharwal commented 2 weeks ago

I'm happy to give it a shot, but defining these methods with a Union doesn't feel right to me. Semantically the fact that problems, integrators and solutions all support getindex is completely separate (they're four different interfaces) and basically just a coincidence.

Also, setindex! for problems uses ___internal_setindex! for reasons I'm not aware of, so we'd just end up merging time-independent solutions and DEIntegrator.

TorkelE commented 2 weeks ago

The tests have been quite heavily modified, so I cannot really tell if they are the same or no. If you say they are the same I trust you.

ChrisRackauckas commented 1 week ago

How is this going?

AayushSabharwal commented 1 week ago

The added tests pass locally last I checked, but the core CI is failing due to an error I haven't been able to reproduce yet (even with --check-bounds=yes and --depwarn=yes). That code hasn't been touched, so the sudden failure is weird.

In fact, even in downstream CI all the depwarns are erroring. I guess tests are now running with --depwarn=yes for some reason?