Skip to content

Conversation

DhairyaLGandhi
Copy link
Member

@DhairyaLGandhi DhairyaLGandhi commented Aug 7, 2025

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

Ref #3859

Add any other context about the problem here.

@DhairyaLGandhi DhairyaLGandhi marked this pull request as draft August 7, 2025 13:50
@@ -198,10 +199,11 @@ function generate_control_function(sys::AbstractSystem, inputs = unbound_inputs(
simplify = false,
eval_expression = false,
eval_module = @__MODULE__,
split = true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing this argument with default true is likely breaking considering that the existing default in mtkcompile is false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked against MTK for the default.

sys::AbstractSystem; additional_passes = [], simplify = false, split = true,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I must have confused the meaning of split in my head, sorry about the noise then :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I dislike having multiple places where a default is defined/ repeated since that can be error prone)

I don't know what pattern mtk follows for these cases if it has one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split=true is whether to use MTKParameters while putting everything into one vector. This has been the default for a bit now.

@DhairyaLGandhi DhairyaLGandhi marked this pull request as ready for review August 8, 2025 05:58
@DhairyaLGandhi
Copy link
Member Author

Cc @AayushSabharwal

@AayushSabharwal AayushSabharwal merged commit 32b8bff into SciML:master Aug 8, 2025
44 of 51 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.

4 participants