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

Docs documenter #47

Closed
wants to merge 0 commits into from
Closed

Conversation

blas-ko
Copy link
Contributor

@blas-ko blas-ko commented Apr 29, 2016

Perdón que cerré el Pull Request pasado.

Documenté todo el Taylor1.jlincluyendo las relaciones de recurrencia para los coeficientes de HomogCoefs. Se utilizaron caracteres de UNICODE ya que Documenter.jl aún no funciona bien con ecuaciones para la documentación interna.

@lbenet
Copy link
Member

lbenet commented Apr 29, 2016

No problem, but use English here. I'll check your PR in the next few days.

@coveralls
Copy link

coveralls commented Apr 29, 2016

Coverage Status

Coverage remained the same at 97.505% when pulling 2d20993 on blas-ko:docs_documenter into c836e07 on JuliaDiff:docs_documenter.

@lbenet
Copy link
Member

lbenet commented May 2, 2016

As we discussed, I suggest:

  • Use one line imperative statement about the function, and in another paragraph provide details,
  • Use TeX format for formulas instead of UTF-strings

@coveralls
Copy link

coveralls commented May 3, 2016

Coverage Status

Coverage remained the same at 97.505% when pulling 6df15e7 on blas-ko:docs_documenter into d825d69 on JuliaDiff:docs_documenter.

@blas-ko
Copy link
Contributor Author

blas-ko commented May 5, 2016

Changes about the suggestions were made.
Docstrings for Taylor1.jl were all included in a src/helpDB.jl script which, by the way, I'm not sure if it's the best place to put it.

Expansion coefficients for the HomogCoefare now in LateX format which can be seen nicely in a notebook but not in the REPL.

@coveralls
Copy link

coveralls commented May 5, 2016

Coverage Status

Coverage decreased (-10.4%) to 87.116% when pulling 9ee5c68 on blas-ko:docs_documenter into d825d69 on JuliaDiff:docs_documenter.

@coveralls
Copy link

coveralls commented May 8, 2016

Coverage Status

Coverage decreased (-0.003%) to 97.503% when pulling 587c602 on blas-ko:docs_documenter into d825d69 on JuliaDiff:docs_documenter.

@coveralls
Copy link

coveralls commented May 8, 2016

Coverage Status

Coverage decreased (-0.003%) to 97.503% when pulling 284d164 on blas-ko:docs_documenter into bae1493 on JuliaDiff:docs_documenter.

@lbenet
Copy link
Member

lbenet commented May 10, 2016

Thanks for everything!

I have few comments on the style:

  • Use imperative form, i.e., "Compute" instead of "Computes".
  • When referring to functions, I think it is better to use latex-style; check what you get for ? exp (in Base they use latex instead of markdown).
  • I would avoid any html commands such as <br> or <center>, as well as intermediate empty lines (before and after the equations); they are not nicely formatted.
  • There are some commits which are not needed; I will clean those before merging.

I think you can add to helpDB.jl what already exists in TaylorN.jl (and in the other files).

@dpsanders
Copy link
Contributor

Is there a particular reason why you chose to have a separate helpDB.jl file? This is something that will be gradually removed from Base Julia. I would recommend instead putting the docstrings together with their functions.

taylor1_variable(T, [order=1])
taylor1_variable([order=1])

Short-cut to define the independent variable as a `Taylor1` polynomial of
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer shortcut to short-cut

@lbenet
Copy link
Member

lbenet commented May 10, 2016

Is there a particular reason why you chose to have a separate helpDB.jl file? This is something that will be gradually removed from Base Julia. I would recommend instead putting the docstrings together with their functions.

That was a suggestion I made to Blas based on what is (currently) done in Base (though i think helpdb.jl is a nicer name, without the "DB" in capitals); I think Blas disliked this idea.
What I do not like of having the docstrings together with the code is that Julia internally (re)orders them in a way which I'd like to control (and do not know how). For instance, ? get_coeff currently display the methods for TaylorN then for HomogeneousPolynomial and finally for Taylor1. I must admit that what I like of having the docstrings together with the code is precisely the essence of literate programing...

If you are strongly in favor of having them together with the code, I think this is the good time to revert things and clean up some commits. (Sorry Blas about thist!)

@dpsanders
Copy link
Contributor

Yes, I'm definitely strongly in favour of having them together with the corresponding functions in the code; as you say, this is indeed literate programming.

As regards the order of appearence of the functions when you do ? get_coeff, this is a good question, and something that should probably be raised on the Julia-users list or as an issue in the main Julia repo.

@lbenet
Copy link
Member

lbenet commented May 10, 2016

@blas-ko My mistake, Blas...

This is indeed in the spirit of literate programing, but not quite... But that's another thing.

@dpsanders
Copy link
Contributor

According to https://groups.google.com/forum/m/#!topic/julia-users/WPHhQtVxCVs
In Julia 0.5, methods in documentation will be listed in order if their definition.

@lbenet
Copy link
Member

lbenet commented May 12, 2016

Thanks for the info!

@blas-ko
Copy link
Contributor Author

blas-ko commented May 16, 2016

I moved back the docstrings into the Taylor1.jl script. If everything's okay, I'll work on documentation for TaylorN.jl and delete helpDB.jl so we can merge.

@lbenet
Copy link
Member

lbenet commented May 16, 2016

Thanks for pushing an updated version. I will correct the docstrings and clean up a little bit this PR, because there is a bunch of commits which do not provide anything. If I can't keep the authorship of the relevant commits, you will clean up the commits.

@lbenet
Copy link
Member

lbenet commented May 16, 2016

Checking today the last commit by @blas-ko , I noticed that I never responded to this and this comments. The question is:

What is the difference between diffTaylor and deriv then? Can they somehow be combined into derivative?

  • diffTaylor returns either a Taylor1 or a TaylorN polynomial, which correspond to the derivative or partial derivative with respect to a given variable. I think we could call this differential.
  • deriv returns the value of the n-th derivative of a Taylor1 polynomial. (Something alike could be implemented for generic partial derivatives of TaylorN polynomials.) In Base, there is diff, which returns the finite difference operator. I think we should stick to deriv.

@dpsanders You are good at naming functions. How do you like this proposal?

@lbenet
Copy link
Member

lbenet commented May 16, 2016

Once we decide this, I'll change the code, the docs and the tests accordingly.

@dpsanders
Copy link
Contributor

Can't they both be called derivative? derivative(p) is the polynomial, and derivative(p, a) is the derivative evaluated at the point a?

@lbenet
Copy link
Member

lbenet commented May 16, 2016

Currently, we have diffTaylor(p) and diffTaylor(p,[r=1::Int]) for Taylor1 and TaylorN; the former returns the Taylor1 polynomial of the derivative while the latter returns the TaylorN polynomial of the (partial) derivative with respect to the r variable. deriv(p, n::Int) for a Taylor1 polynomial, returns the value of the n-th derivative (of the Taylor expansion at the value around which the expansion is made).

My point is that it makes things ambiguous if we use the same name to return different quantities (polynomial of the differential vs value of a given derivative). Moreover, we could define diffTaylor(p::Taylor1,[r=1::Int]) to return the polynomial corresponding to the r-th derivative, which would collide with deriv(p,n), if we use the same names.

What about diffTaylor be differential and deriv be derivative?

@dpsanders
Copy link
Contributor

I would find that very confusing. I suggest derivative for both (the Julian style :) ) but change the order of the arguments, so derivative(i, p) is the ith derivative and derivative(p, a) is evaluated at a. And derivative(i, P, a) is the ith derivative evaluated at a!

@dpsanders
Copy link
Contributor

Note that all tests are failing on travis.

@lbenet
Copy link
Member

lbenet commented May 17, 2016

I like derivative for all possibilities, with the subtlety of the order! I'll implement this later in a different PR, so we are not stepping in our toes.

@coveralls
Copy link

coveralls commented May 22, 2016

Coverage Status

Coverage remained the same at 97.505% when pulling 9863724 on blas-ko:docs_documenter into bae1493 on JuliaDiff:docs_documenter.

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