Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use getu/setu for all indexing #721

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

AayushSabharwal
Copy link
Member

@AayushSabharwal AayushSabharwal commented Jun 17, 2024

@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

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.

@AayushSabharwal
Copy link
Member Author

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

@oscardssmith
Copy link
Contributor

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

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Jun 17, 2024

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
Copy link
Member

TorkelE commented Jun 17, 2024

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
Copy link
Member

How is this going?

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Jun 23, 2024

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?

@ChrisRackauckas ChrisRackauckas merged commit 2a3e88d into SciML:master Jun 26, 2024
29 of 40 checks passed
@AayushSabharwal AayushSabharwal deleted the as/getu-everywhere branch June 26, 2024 15:49
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.

4 participants