Skip to content

Conversation

@HarrisonGrodin
Copy link
Contributor

@HarrisonGrodin HarrisonGrodin commented Feb 14, 2019

Closes #99, closes #100, closes #101.

@HarrisonGrodin
Copy link
Contributor Author

HarrisonGrodin commented Feb 14, 2019

It seems like we should try to be consistent with how we shorten macro names. Some ideas:

# Option A
@param t σ
@var x(t) y(t)
@deriv D'~t
# Option B
@parameter t σ
@variable x(t) y(t)
@derivative D'~t D''~t

We could also pluralize. Thoughts? @ChrisRackauckas @dpsanders

@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #103 into master will increase coverage by 1.26%.
The diff coverage is 82.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   92.02%   93.29%   +1.26%     
==========================================
  Files          11       11              
  Lines         326      328       +2     
==========================================
+ Hits          300      306       +6     
+ Misses         26       22       -4
Impacted Files Coverage Δ
src/ModelingToolkit.jl 75% <ø> (ø) ⬆️
src/utils.jl 87.5% <ø> (ø) ⬆️
src/function_registration.jl 100% <100%> (ø) ⬆️
src/systems/diffeqs/first_order_transform.jl 100% <100%> (ø) ⬆️
src/systems/diffeqs/diffeqsystem.jl 94.23% <100%> (ø) ⬆️
src/simplify.jl 95% <100%> (ø) ⬆️
src/differentials.jl 97.72% <100%> (ø) ⬆️
src/equations.jl 92% <50%> (ø) ⬆️
src/variables.jl 86.48% <66.66%> (+1.87%) ⬆️
src/operations.jl 92.85% <83.33%> (+21.42%) ⬆️

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 02edf04...6379485. Read the comment docs.

@ChrisRackauckas
Copy link
Member

I think Option A is fine, but I think @dpsanders likes the full names?

@HarrisonGrodin HarrisonGrodin mentioned this pull request Feb 19, 2019
@HarrisonGrodin
Copy link
Contributor Author

Whoa, this has a version-based bug - works on 1.1/nightly, but not on 1.0.

@HarrisonGrodin
Copy link
Contributor Author

The more I look at the options, I like the full names (Option B) - since these lines will only be typed a few times at the beginning of each model, it seems reasonable to make the names slightly more verbose/clear. I also think pluralizing will help with this goal.

@parameters t σ
@variables x(t) y(t)
@derivatives D'~t D''~t

@ChrisRackauckas Am I good to change this?

@ChrisRackauckas
Copy link
Member

Yeah that looks good.

@HarrisonGrodin
Copy link
Contributor Author

Update: the reason 1.1 works (while 1.0 fails) seems to be due to JuliaLang/julia#29668. This does beg the question of whether or not we should have comparison functions return non-booleans (outside of an explicit context), though.

@ChrisRackauckas ChrisRackauckas merged commit ab45092 into master Mar 8, 2019
@ChrisRackauckas ChrisRackauckas deleted the hg/fix/cleanup branch March 8, 2019 21:52
@ChrisRackauckas
Copy link
Member

What booleans would they return?

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.

Parsing of <= Don't use capitals for macros Use "variable" instead of "unknown"

3 participants