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

Restore error when unknowns in the initialization system don't have guesses #2877

Merged
merged 5 commits into from
Jul 20, 2024

Conversation

hersle
Copy link
Contributor

@hersle hersle commented Jul 19, 2024

Another attempt to fix #2873 in a simpler way; supersedes #2874.

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

@hersle
Copy link
Contributor Author

hersle commented Jul 19, 2024

I don't understand what's going on with the DDE test.

Seems weird that just disabling one early-return optimization causes one test to fail. Looks like there is just a t that is not substituted. Is there something DDE-specific going on here, or have DDEs just not gotten the non-autonomous treatment?

@ChrisRackauckas
Copy link
Member

Looks like there is just a t that is not substituted. Is there something DDE-specific going on here, or have DDEs just not gotten the non-autonomous treatment?

DDEs naturally do a bit more with t since it's common to have say x(t - tau), or x(t - y(t)). I wonder if those cases trip it up.

@ChrisRackauckas
Copy link
Member

This looks good to go though? Just the latexify regression test needs an update, but I'd be fine handling that after merge.

@ChrisRackauckas ChrisRackauckas merged commit c6a9ee9 into SciML:master Jul 20, 2024
21 of 22 checks passed
@hersle
Copy link
Contributor Author

hersle commented Jul 20, 2024

Yes, that's what I realized. It seems that DDEs relied on the early return, which is why they pass tofloat = false in the first place.

I think it's good to merge, yes.

@ChrisRackauckas
Copy link
Member

Thanks for seeing this one through. I hadn't found the time to do it since it was pretty non-trivial, but in the end I think you figured out a good solution.

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.

Helpful error for missing guesses has disappeared
2 participants