Skip to content

Conversation

EduardoMartinezLopez
Copy link

@EduardoMartinezLopez EduardoMartinezLopez commented Sep 18, 2025

This PR Fixes #3934

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.

@EduardoMartinezLopez EduardoMartinezLopez marked this pull request as draft September 18, 2025 23:16
@EduardoMartinezLopez EduardoMartinezLopez marked this pull request as ready for review September 18, 2025 23:23
@ChrisRackauckas
Copy link
Member

This reformats the whole repo. Right now the formatter is broken so don't worry about it. Revert that.

…h any of these files except ext/MTKFMIExt.jl!"

This reverts commit 37c6897.
@EduardoMartinezLopez
Copy link
Author

@ChrisRackauckas Thanks! I reverted the formatter commit.

@EduardoMartinezLopez
Copy link
Author

EduardoMartinezLopez commented Sep 18, 2025

@ChrisRackauckas Question for you. It seems like the existing code in MTKFMIExt > parseFMIVariableName() mutates the "name" input variable. Is that OK? My understanding is that this is considered bad practice as Julia passes by reference. Also if it the input is mutated, the convention is to add ! to the function name, right?

@ChrisRackauckas
Copy link
Member

I don't see where it mutates it. It creates a view but then creates a Symbol, which is effectively an interned string. But I don't see it actually mutating the string? Put to the line that you're worried about.

Though it probably shouldn't create the symbol there for performance reasons on large models, but that's a minor detail.

@EduardoMartinezLopez
Copy link
Author

Ok, I was concerned about the name = @view name[....] with name on the left side of an assignment. I guess that's ok with @view macro?

@ChrisRackauckas
Copy link
Member

that just makes name be a reference to a subset of what was previously name, no mutation there.

@AayushSabharwal
Copy link
Member

The test doesn't work

@EduardoMartinezLopez
Copy link
Author

@AayushSabharwal what is the problem with the test on your end?

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.

Cannot create MTK model from FMU with multi-dimensional variables with derivatives
3 participants