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

fix: change variable defaults to guesses #281

Closed

Conversation

avinashresearch1
Copy link
Contributor

@avinashresearch1 avinashresearch1 commented Apr 11, 2024

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.

@avinashresearch1 avinashresearch1 changed the title fix: change variable defaults to guesses in Thermal fix: change variable defaults to guesses Apr 11, 2024
Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 31 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (3a8fb0a) to head (6e2af95).
Report is 7 commits behind head on main.

Current head 6e2af95 differs from pull request most recent head f000b3d

Please upload reports for the commit f000b3d to get more accurate results.

Files Patch % Lines
src/Blocks/utils.jl 0.00% 29 Missing ⚠️
src/Blocks/continuous.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #281       +/-   ##
==========================================
- Coverage   61.48%   0.00%   -61.49%     
==========================================
  Files          46      32       -14     
  Lines        1472    1657      +185     
==========================================
- Hits          905       0      -905     
- Misses        567    1657     +1090     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avinashresearch1
Copy link
Contributor Author

avinashresearch1 commented Apr 11, 2024

@ven-k I'm a little confused as to what is causing the test failure, its from the code below:

@mtkmodel SISO begin
           @parameters begin
               u_start = 0.0
               y_start = 0.0
           end
           @variables begin
               u(t), [guess = u_start, description = "Input of SISO system"]
               y(t), [guess = y_start, description = "Output of SISO system"]
           end
           @components begin
               input = RealInput(u_start = u_start)
               output = RealOutput(u_start = y_start)
           end
           @equations begin
               u ~ input.u
               y ~ output.u
           end
       end

its due to guess = u_start, not sure why it says u_start not defined when I'm trying to use the one in the params (I'm not extremely familiar with the @mtkmodel syntax).

@avinashresearch1
Copy link
Contributor Author

@ven-k I'm a little confused as to what is causing the test failure, its from the code below:

@mtkmodel SISO begin
           @parameters begin
               u_start = 0.0
               y_start = 0.0
           end
           @variables begin
               u(t), [guess = u_start, description = "Input of SISO system"]
               y(t), [guess = y_start, description = "Output of SISO system"]
           end
           @components begin
               input = RealInput(u_start = u_start)
               output = RealOutput(u_start = y_start)
           end
           @equations begin
               u ~ input.u
               y ~ output.u
           end
       end

its due to guess = u_start, not sure why it says u_start not defined when I'm trying to use the one in the params (I'm not extremely familiar with the @mtkmodel syntax).

This requires: SciML/ModelingToolkit.jl#2501

@avinashresearch1 avinashresearch1 marked this pull request as draft April 12, 2024 22:19
@avinashresearch1 avinashresearch1 force-pushed the avi/defaults_to_guesses branch from 8276b01 to b54c7b1 Compare May 3, 2024 12:31
@avinashresearch1 avinashresearch1 marked this pull request as ready for review May 3, 2024 12:34
@avinashresearch1
Copy link
Contributor Author

Rebased after @AayushSabharwal 's changes #291

@avinashresearch1
Copy link
Contributor Author

@baggepinnen request for review

@@ -20,7 +20,7 @@ Initial value of integrator state ``x`` can be set with `x`
@mtkmodel Integrator begin
@extend u, y = siso = SISO()
@variables begin
x(t) = 0.0, [description = "State of Integrator"]
x(t), [guess = 0.0, description = "State of Integrator"]
Copy link
Contributor

@SebastianM-C SebastianM-C May 6, 2024

Choose a reason for hiding this comment

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

I also had the same suggestion in #292, but #292 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Integrators always start at zero though?

Most of the time this is what users want, but it is occasionally useful to have the system solve for an integrator value that makes an initial control signal have a particular value to avoid initial transients, so forcing the user to specify the initial state explicitly might not be too bad

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the user can not easily remove a default value that has been specified in the library? If they cannot remove the value, we must not provide it since then it cannot be solved for

Copy link
Contributor

@baggepinnen baggepinnen Sep 23, 2024

Choose a reason for hiding this comment

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

Since we now have the capability to remove a default, the explicit state variables, like those in Integrator, Derivative, FirstOrder, SecondOrder should keep their defaults, but y,u in SISO etc. should not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am probably going to make a new PR, since almost everything in this is not correct :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChrisRackauckas We can probably close this without merging since this PR has more incorrect than correct things, I'll make a new PR for the things that I would like to fix and the docs.

Copy link
Member

Choose a reason for hiding this comment

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

okay

@baggepinnen baggepinnen reopened this Jun 10, 2024
@ChrisRackauckas
Copy link
Member

Where is this at?

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