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

Adding @constants #1821

Merged
merged 33 commits into from
Nov 3, 2022
Merged

Adding @constants #1821

merged 33 commits into from
Nov 3, 2022

Conversation

lamorton
Copy link
Contributor

@lamorton lamorton commented Sep 16, 2022

See #1802. The basic idea is:

  • Create new @constants that are like @parameters but must have a default value
  • Constants are not name-spaced, they are assumed to be globally unique (ex, μ₀ for permeability of free space doesn't belong to any particular model).
  • Constants are collected from equations during generate_function & their definitions (equating the symbol to its default value) are injected into the preface of the function.

So far only ODESystems have been equipped to use @constants. Failure to handle them properly will result in an UndefVar error when attempting to call a generated function.

Initially I tried treating constants similarly to parameters: adding a cs field to the system & renamespacing them when flattening systems. However, adding a new field was breaking lots of tests b/c of the constructor signature change. If it is decided that we should go back this way, maybe making an equivalent to NullParameters so that there can be a default value for cs and making that argument to the constructor optional would be the workaround.

Other approaches I tried for replacing constants with their values:

  • injecting the constant definitions into the system of equations & using structural_simplify to knock them out
  • Using substitute to replace the constants with their values in the equations prior to building the function

@lamorton
Copy link
Contributor Author

There's a bug in the CodeCov webpage for me: most of the time, when I go to the Project and try to show the dropdown code diffs, I get a spinner for a split-second and then it collapses. It worked once though, but then I refreshed and it's failing again.
I have the problem on both Firefox and Chrome (I'm on Windows 10). I can't debug the CodeCov right now because of this.

image

@lamorton lamorton changed the title Adding @constants WIP: Adding @constants Sep 20, 2022
@lamorton lamorton marked this pull request as draft September 20, 2022 14:31
@lamorton lamorton marked this pull request as ready for review September 24, 2022 00:44
@lamorton
Copy link
Contributor Author

I'm working up some documentation & fixes for the codecov regression. Otherwise it's ready to go.

One feature we'll want to add eventually is support for substituting the constants with their literal values at the beginning of structural_simplify. This will allow further simplifications that are only possible with literals rather than symbols (eg, solution of linear equations that involve dividing by a constant).

@isaacsas
Copy link
Member

Does this handle updating constants in events too?

@lamorton
Copy link
Contributor Author

@isaacsas No, constants are immutable. If needs to be updated in an event, it should be a parameter, not a constant.

@isaacsas
Copy link
Member

isaacsas commented Sep 24, 2022

Sorry, I meant does the event code need to be updated to work with constants similar to the updates you made across the various system types?

@lamorton
Copy link
Contributor Author

lamorton commented Sep 25, 2022

🤣 Got it now. I'll sprinkle some constants in test/funcaffect.jl and find out. Basically, work is needed in any places where build_function or toexpr are getting used. I think thought I covered my bases by fixing all the generate_callback instances for each system type.

Edit: good catch, there were some build_functions lurking in systems/callbacks.jl that I missed.

@lamorton lamorton changed the title WIP: Adding @constants Adding @constants Sep 26, 2022
@lamorton
Copy link
Contributor Author

@ChrisRackauckas @YingboMa The DataDrivenDiffEq failure is an upstream bug. I think the invalidations failure is also an intermittent bug with SnoopCompile. I think this is ready for review.

@ChrisRackauckas
Copy link
Member

Yes the DataDrivenDiffEq upstream issue is known.

@lamorton
Copy link
Contributor Author

@ChrisRackauckas Is the Invalidation failure not also a known issue? Same thing happened to Yingbo's PR and another one.

@ChrisRackauckas
Copy link
Member

Yes it seems to be hyperactive.

@lamorton
Copy link
Contributor Author

@ChrisRackauckas @YingboMa anything else you'd like to see prior to a review?

@YingboMa
Copy link
Member

Thanks a lot! I will review this tomorrow.

@YingboMa
Copy link
Member

YingboMa commented Nov 3, 2022

Sorry for the late review. I just pushed some modifications to the PR.

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.

None yet

4 participants