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

Start on API documentation #125

Merged
merged 4 commits into from
Jun 23, 2019
Merged

Start on API documentation #125

merged 4 commits into from
Jun 23, 2019

Conversation

jlumpe
Copy link
Contributor

@jlumpe jlumpe commented Apr 26, 2019

Added basic docstrings to most of the core types and a few functions. Created documentation skeleton with API page showing docstrings.

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #125 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
- Coverage    94.3%   94.26%   -0.05%     
==========================================
  Files          11       11              
  Lines         369      366       -3     
==========================================
- Hits          348      345       -3     
  Misses         21       21
Impacted Files Coverage Δ
src/ModelingToolkit.jl 75% <ø> (ø) ⬆️
src/function_registration.jl 100% <ø> (ø) ⬆️
src/systems/nonlinear/nonlinear_system.jl 93.93% <ø> (ø) ⬆️
src/equations.jl 60% <100%> (ø) ⬆️
src/operations.jl 71.42% <100%> (ø) ⬆️
src/variables.jl 91.42% <100%> (ø) ⬆️
src/systems/diffeqs/diffeqsystem.jl 100% <100%> (ø) ⬆️
src/differentials.jl 95.34% <100%> (ø) ⬆️

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 cc5df4c...978aca7. Read the comment docs.

Copy link
Contributor

@HarrisonGrodin HarrisonGrodin left a comment

Choose a reason for hiding this comment

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

Although it might seem counterintuitive at first, I'd actually rather that most type definitions/fields not be included in the documentation, instead opting towards a more API-based approach. For example, rather than telling the user externally that there are fields for storing the independent variables, dependent variables, parameters, etc., I'd rather document the accessor functions:

https://github.com/JuliaDiffEq/ModelingToolkit.jl/blob/5c8deea4a4de4a0f39bdb6a2a4ddedce75657fc3/src/ModelingToolkit.jl#L26-L28

Similarly, rather than guaranteeing that a cached version of the Jacobian is available in the jac field, I'd much rather just guarantee that calculate_jacobian returns the right result. The user doesn't need to care about how the Jacobian is calculated, or even that it's cached - instead, all we have to promise is that calculate_jacobian does the right thing.

This mindset is based on the recent explicit split between interface and implementation; within ModelingToolkit, we're making an effort to make no guarantees about the internal implementation, instead giving users ways of using the library without respect to implementation. This allows us to retain the same API while making our internals work differently.

Overall, though, I'm very excited about this PR! It'll be great to have documentation around ModelingToolkit.

@jlumpe
Copy link
Contributor Author

jlumpe commented Apr 26, 2019

@HarrisonGrodin makes sense, I've removed the documentation for all fields which seem to have accessors. Those were ODESystem and NonlinearSystem (except for .exprs), and Constant.value (which is available through Base.get()).

@jlumpe
Copy link
Contributor Author

jlumpe commented May 13, 2019

@HarrisonGrodin the most recent build passed but it didn't seem to update here. This should be ready to merge.

@HarrisonGrodin
Copy link
Contributor

It seems like there are still some TODOs?

@jlumpe
Copy link
Contributor Author

jlumpe commented May 14, 2019

I actually intentionally left those in for other developers who know more about how the package works than I do. I can remove them if you don't want them in master.

@jlumpe
Copy link
Contributor Author

jlumpe commented Jun 23, 2019

@HarrisonGrodin merge conflicts resolved, should I remove the TODOs or leave them in?
\

@ChrisRackauckas
Copy link
Member

The hold up is really that we don't know what the public API really should be yet... but screw it. Let's merge and keep refining it.

@ChrisRackauckas ChrisRackauckas merged commit de06aae into SciML:master Jun 23, 2019
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.

3 participants