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

Math repeated in documentation #123

Open
inducer opened this issue Oct 23, 2020 · 4 comments
Open

Math repeated in documentation #123

inducer opened this issue Oct 23, 2020 · 4 comments

Comments

@inducer
Copy link
Contributor

inducer commented Oct 23, 2020

In mirgecom.euler, the documentation often restates, in math notation, what the code does. I think that's redundant. By including the sphinx.ext.viewcode extension,we can instead let the docs have convenient links to source code #122.

Edit: Also in initializers.

@MTCam @lukeolson @majosm @anderson2981 Opinions?

@MTCam
Copy link
Member

MTCam commented Oct 24, 2020

The inline docs come up in my editor when I'm browsing code. I browse and read the code almost exclusively and almost never look at the full rendered documentation. It is convenient for me to see the details right in the docstrings, and a loathsome ordeal when I find myself digging through documentation for the part of the model that a given routine implements.

It's great that the doc links to the routines and all, but far more often I am in the situation where I already know the routine, and am actively looking at the code I need to understand - and having the math right there is nice.

That's just like .. my opinion, man.

@inducer
Copy link
Contributor Author

inducer commented Oct 24, 2020

see the details right in the docstrings

If the docstrings genuinely add something, I'm not opposed, of course. But this is an example where I find the value less clear:

mirgecom/mirgecom/euler.py

Lines 152 to 163 in 37b10a0

The inviscid fluxes are
:math:`(\rho\vec{V},(\rhoE+p)\vec{V},\rho(\vec{V}\otimes\vec{V})+p\mathbf{I})`
"""
dim = discr.dim
cv = split_conserved(dim, q)
p = eos.pressure(cv)
mom = cv.momentum
return join_conserved(dim,
mass=mom,
energy=mom * scalar((cv.energy + p) / cv.mass),
momentum=np.outer(mom, mom)/scalar(cv.mass) + np.eye(dim)*scalar(p))

There's the TeX formula, and then there's Python formula. They say exactly the same thing (or at least they're supposed to), which makes the information redundantly represented. Personally, I find the Python easier to read than the TeX. Given that they're redundant, for me that begs the question: Why have both?

That's just like .. my opinion, man.

Same the other way around. I care. I appreciate that you care enough to respond. Maybe we can find a way that we can both live with.

@MTCam
Copy link
Member

MTCam commented Oct 25, 2020

I agree with the spirit of what you are saying about whether the documentation adds something useful. Over time, I imagine that the Python expression will be more useful - but currently even though the formula is in tex, it is more readily deciphered (by me) than the Python.

The presence of the formula here doesn't (seem to) actively detract from the intent of the documentation, and since it is added value at least for someone, I'd argue that it adds something "useful," even if redundant. Redundancy isn't always a sin.

Sure, I can go look up exactly what np.outer, scalar, np.eye actually do and any required details of Python operators - but if the formula is sitting there, then I don't need to.

@inducer
Copy link
Contributor Author

inducer commented Oct 25, 2020

Sure, I can go look up exactly what np.outer, scalar, np.eye actually do and any required details of Python operators - but if the formula is sitting there, then I don't need to.

I can see that point of view, and it sort of explains our difference in perspective. I propose we leave things be as they are for now and revisit this in a year.

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

No branches or pull requests

2 participants