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

[WIP/RFC] Refactor IR around Terms.jl #76

Closed
wants to merge 5 commits into from

Conversation

HarrisonGrodin
Copy link
Contributor

@HarrisonGrodin HarrisonGrodin commented Jan 9, 2019

Terms.jl will mostly replace the IR structure (e.g. Operation), leaving other data structures (e.g. Variable) the same until future PRs.

TODO:

  • Swap Operation for Term
  • Swap Constant(x) for @term(x)
  • Update algorithms
  • Update README

@HarrisonGrodin HarrisonGrodin added the enhancement New feature or request label Jan 9, 2019
@codecov
Copy link

codecov bot commented Jan 9, 2019

Codecov Report

Merging #76 into master will decrease coverage by 0.53%.
The diff coverage is 82.05%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #76      +/-   ##
=========================================
- Coverage   80.43%   79.9%   -0.54%     
=========================================
  Files          11      11              
  Lines         414     413       -1     
=========================================
- Hits          333     330       -3     
- Misses         81      83       +2
Impacted Files Coverage Δ
src/ModelingToolkit.jl 75% <ø> (ø) ⬆️
src/variables.jl 72.28% <ø> (-1.05%) ⬇️
src/systems/diffeqs/diffeqsystem.jl 82.92% <100%> (ø) ⬆️
src/systems/nonlinear/nonlinear_system.jl 96.96% <100%> (ø) ⬆️
src/differentials.jl 79.54% <100%> (ø) ⬆️
src/operations.jl 70% <50%> (-5.87%) ⬇️
src/utils.jl 82.97% <80.64%> (-5.26%) ⬇️
src/function_registration.jl 0% <0%> (-22.23%) ⬇️

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 829e2e4...cf06d08. Read the comment docs.

@HarrisonGrodin
Copy link
Contributor Author

All simple features now work using Term; the more in-depth procedures (e.g. system construction) still need to be translated.

@HarrisonGrodin
Copy link
Contributor Author

Closing this PR and bringing back changes in smarter, more targeted PRs. Although it initially seemed that exposing the use of Terms.jl would be ideal, it soon became clear that this is hard to use and develop with.

After making some of the more structural changes that accidentally became part of this, I'll open a PR which only changes the internals of Operation, ideally without changing the API. This leaves us the benefit of speed and compatibility with other Terms-based packages without requiring direct use of the abstract viewpoint taken in Terms.

@HarrisonGrodin HarrisonGrodin deleted the hg/feature/terms branch January 10, 2019 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant