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

Refactor Constant representation #79

Merged
merged 5 commits into from Jan 11, 2019
Merged

Refactor Constant representation #79

merged 5 commits into from Jan 11, 2019

Conversation

HarrisonGrodin
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jan 11, 2019

Codecov Report

Merging #79 into master will decrease coverage by 0.23%.
The diff coverage is 54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
- Coverage   79.46%   79.23%   -0.24%     
==========================================
  Files          11       11              
  Lines         409      390      -19     
==========================================
- Hits          325      309      -16     
+ Misses         84       81       -3
Impacted Files Coverage Δ
src/utils.jl 75% <0%> (-4.17%) ⬇️
src/differentials.jl 79.54% <0%> (ø) ⬆️
src/systems/diffeqs/first_order_transform.jl 89.13% <100%> (+1.89%) ⬆️
src/operations.jl 63.15% <40%> (-6.85%) ⬇️
src/ModelingToolkit.jl 50% <50%> (-25%) ⬇️
src/variables.jl 72.09% <57.14%> (-1.25%) ⬇️
src/simplify.jl 90% <75%> (+3.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6061f03...9bff84f. Read the comment docs.

@@ -5,7 +5,7 @@ function lower_varname(var::Variable, naming_scheme; lower=false)
lower_varname(var.name, D.x, order, var.subtype, naming_scheme)
end
function lower_varname(sym::Symbol, idv, order::Int, subtype::Symbol, naming_scheme)
order == 0 && return Variable(sym, subtype)
order == 0 && return Variable(sym, subtype=subtype)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What a subtle bug!

Copy link
Member

Choose a reason for hiding this comment

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

yeah...

Copy link
Member

Choose a reason for hiding this comment

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

does that fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - I also changed this function afterwards, to make the goal more explicit.

@ChrisRackauckas
Copy link
Member

How does this effect the Terms usage? In terms, would a Constant be a separate entity as well?

@HarrisonGrodin
Copy link
Contributor Author

HarrisonGrodin commented Jan 11, 2019

Reasoning about algebraic expressions as syntax trees, Terms deals with the "branches", leaving the "leaves" to the user. My plan is to strategically swap out the internals of Operation for a Term, keeping the interface similar without requiring many other changes. The use of Constants should be completely compatible with Terms, in the same way that having a custom variable representation will be. (Additionally, since the main purpose Constant is for dispatch/subtyping purposes, it's possible that it could be removed later, once everything is stored as abstract terms.)

This particular refactor is part of a general move towards structuring our objects in a way that more naturally describes the data stored. Since the concept of a numerical constant seems fairly disconnected from the concept of a variable/parameter, splitting it off makes a handful of undesired states unrepresentable. By doing this, I eliminated at least one bug immediately which was previously able to sneak by.

@ChrisRackauckas ChrisRackauckas merged commit 0097578 into master Jan 11, 2019
@ChrisRackauckas ChrisRackauckas deleted the hg/fix/constant branch January 11, 2019 07:18
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

2 participants