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

[RFC] Remove more unnecessary defaults #306

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Conversation

ChrisRackauckas
Copy link
Member

No description provided.

@ChrisRackauckas
Copy link
Member Author

@avinashresearch1 @baggepinnen @bradcarman these are all of the =0 defaults that remain in the standard library. Which of these should be kept? I think the ones on the sensors should remain.

@ven-k @AayushSabharwal I think we need a feature where a default =nothing simply drops it.

@baggepinnen
Copy link
Contributor

baggepinnen commented Jul 9, 2024

If a default is provided in this library, how does a user indicate that they want to make this a free variable during initialization, should they want to? Is this the var => nothing override suggested above?

@bradcarman
Copy link
Contributor

All I'm going to schedule a workshop for us to sort this out. I've tried and failed to get a proper initialization working and I'd like us all to go thru this together to sort out how it's all supposed to work.

@ChrisRackauckas
Copy link
Member Author

Is this the var => nothing override suggested above?

Yes. But I think in general we shouldn't add any defaults where it would be common to do that override.

@baggepinnen
Copy link
Contributor

A somewhat common usecase where the default is unwanted is when trying to initialize a model such that some derivatives are zero. For example, Integrators often start at 0, but having a zero integration state in, e.g., a PID controller often leads to an initial transient where the controller must find the integral value that eliminates the control error. If you want to start the simulation without this transient, you must allow the initializer to find the integrator state.

I think the ones on the sensors should remain.

Sensor outputs should typically start at exactly the same value as the thing they are measuring, not 0?

@ChrisRackauckas
Copy link
Member Author

Sensor outputs should typically start at exactly the same value as the thing they are measuring, not 0?

Oh true.

@ChrisRackauckas
Copy link
Member Author

For example, Integrators often start at 0, but having a zero integration state in, e.g., a PID controller often leads to an initial transient where the controller must find the integral value that eliminates the control error. If you want to start the simulation without this transient, you must allow the initializer to find the integrator state.

Would you argue the default should still be a 0 on the integrator, and someone should flip that to nothing, or should a user always have to provide it?

@baggepinnen
Copy link
Contributor

Would you argue the default should still be a 0 on the integrator, and someone should flip that to nothing, or should a user always have to provide it?

I'm torn, I could argue both ways. I'm leaning slightly towards letting the default be 0 to improve ergonomics in the most common use case, and let the more advanced use case require the extra step. The question then is, how do we make sure that the user understands that a default provided in the library is adding a perhaps unwanted equation? Could the error message/warning for over-constrained initialization systems include a hint at removing defaults with nothing?

@ChrisRackauckas
Copy link
Member Author

Could the error message/warning for over-constrained initialization systems include a hint at removing defaults with nothing?

It could, yes. It think it's hard to point to which equations the issue might be though, you can just say that someone likely put an initial condition that shouldn't be there, and at most we could print out the full list of initial conditions... but that could be long.

@mtiller-jh
Copy link

mtiller-jh commented Jul 18, 2024

Just to build on what @baggepinnen said. There are use cases like wanting to start a system in equilibrium where the integrator state may need to start in a non-zero value. But there are, of course, cases where you just want to start the integrator start at zero.

The solution that Modelica uses, which I quite like, is to have the component enumerate the different initialization schemes. Modelica doesn't have a good enum type. The JSML enum type is a bit richer (although largely unimplemented). As an example, you could write an integrator like this:

enum IntegratorInitializationOptions = Zero | InitialValue(y0::Real) | Steady

component Integrator
  u = RealInput()
  y = RealOutput()
  parameter init::IntegratorInitliazationOptions = Zero
relations
  switch init
    case Zero:
      initial y = 0
    case InitialValue:
      initial y = init.y0
    case Steady:
      initial der(u) = 0
  end
  if init==Zero
    initial y = 0
  else if init==
  der(y) = u
end

This enum functionality isn't totally implemented, but this gives the idea which is that you can make the behavior conditional on some exhaustive set of options. One thing you can already do today which is the alternative:

component Integrator
  u = RealInput()
  y = RealOutput()
relations
  der(y) = u
end

component InitializedIntegrator
  extends Integrator
  parameter y0::Real
relations
  initial y = 0
end

component ZeroIntegrator
  extends InitializedIntegrator(y0=0)
end

component SteadyIntegrator
  extends Integrator
relations
  initial der(u) = 0
end

This is more aligned with what we discussed with Gary which is to note have super complicated component models that generate super complicated code but rather to specifically use the integrator we really need. If you create a system this way, you can do this:

component GarysModel
  ...
  integrator::Integrator = ZeroIntegrator()
  ...
end

component GarysSteadyModel
  extends GarysModel(integrator = SteadyIntegrator())
end

@baggepinnen what do you think of these options. Similar, I wonder if @bradcarman has any comments on this?

@baggepinnen
Copy link
Contributor

I much favor the different initialization options over the different components. How to initialize an integrator typically depends on the particular scenario is being simulated, it's not a fundamental property of the system being modeled. The same model may thus need different initialization strategies for different simulation runs.

Also, if I have a PI controller in a model library, I don't want to maintain different versions of this depending on how one of the internal state variables should be initialized, neither do I want to mess with "replace" functionality just because I want a state variable to start at a non-zero value.

Lastly, the integrator is the same component in all these cases, having variants of it that only differs in initialization seems confusing to be an leads to a proliferation of component variations.

@mtiller-jh
Copy link

mtiller-jh commented Jul 19, 2024

This is how the Modelica Standard Library addressed this. Ultimately, we can do it either way and any user can do it either way. So for our JSML standard library, we can do it with the enum based approach. I completely agree with you about the flexibility although ultimately it is probably more a UI/UX related issue because doing a replace and changing the value of a parameter isn't going to be that different for the user. But for the creator and maintainer of the component, yes, using parameters to capture the variations seems better.

@AayushSabharwal
Copy link
Member

I think we need a feature where a default =nothing simply drops it.

SciML/ModelingToolkit.jl#2887

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.

5 participants