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

Add docs fixes #128

Merged
merged 8 commits into from
Jul 27, 2023
Merged

Add docs fixes #128

merged 8 commits into from
Jul 27, 2023

Conversation

lukem12345
Copy link
Member

Close #127

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d2b2a79) 88.40% compared to head (7f03237) 88.40%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #128   +/-   ##
=======================================
  Coverage   88.40%   88.40%           
=======================================
  Files           9        9           
  Lines        1009     1009           
=======================================
  Hits          892      892           
  Misses        117      117           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jpfairbanks
Copy link
Member

It looks like this closes just the climate model docs problems, but we still need to update the old examples

@jpfairbanks
Copy link
Member

@lukem12345 ready to merge this?

@lukem12345
Copy link
Member Author

@jpfairbanks Not yet, but I'll push docs changes that I have made. Some of the encoding of boundary conditions in the Poiseuille flow were handled strangely before we settled on current best practices. Also, need to double-check Diffusion-Advection and Boundary Conditions pages

@lukem12345
Copy link
Member Author

Going to push fixes for overview.md and boundary_conditions.md in a few.

@jpfairbanks
Copy link
Member

Nice, I am working on a new docs page using simple equations to go after overview for ASKEM.

@lukem12345
Copy link
Member Author

Great!

On an editorial note:

I am wondering what the value we are getting out of using the star d star d formulation for diffusion is in our docs.

Defining diffusion/ the Laplacian this way:

  • educates readers somewhat on the Exterior Calculus
  • demonstrates the elegance of the DEC
  • it is only a difference of 3 symbols to write one instead of the other
  • the star d star d formulation is nicer if you want to build up simulations using only things you find directly in the de Rham complex
  • not using Delta reduces the number of distinct symbols on the screen

However:
I think it should be used in examples only where the author wishes to educate readers on the definition of the Laplacian:

  • this symbol is already recognizable to readers
  • this is how users write Decapodes in practice
  • via overload resolution, the Laplacian may automatically take the d del + del d form when the argument is a 1 form

I think making this change is a demonstration of the principle that composition should be performed:

  • to reduce cognitive load
  • in anticipation of components to be swapped out
  • to express some physical intuition (e.g. magnetics is separate from hydrodynamics in MHD)
  • for education's sake

Without any of the above reasons, it delves somewhat into composition for composition's sake.

@jpfairbanks
Copy link
Member

I think it would be fine to use Laplacian everywhere we do diffusion and add a new docs page showing composition and aliases. I'll make a new issue to track composite operators. I'd like to get the old docs restored to main ASAP and then address general docs improvement in additional PRs

@jpfairbanks
Copy link
Member

I did the merge conflicts

@lukem12345
Copy link
Member Author

Added in versions of BC and Overview that should be correct barring git pulling and pushing doing odd things.

Poiseuille is partially fixed. The way that boundary conditions happened in the original Poiseuille doc was against current conventions, so this page has taken longer to work through. It will be shorter and clearer after that process is done, which is good.

In the meantime, BC and Overview docs can be merged through whatever git strategies of choice. (cherry-pick overview and bc changes onto a different branch, make a commit that removes changes to Poiseuille in this branch, etc.)

@lukem12345
Copy link
Member Author

Looks like the environment on the GitHub action is slightly different than my local system for whatever reason. Some paths are getting mixed. I'll change some relative paths.

@jpfairbanks
Copy link
Member

I'm going to merge this and if you have more docs updates, they can go in a new PR.

@jpfairbanks jpfairbanks mentioned this pull request Jul 27, 2023
@jpfairbanks jpfairbanks marked this pull request as ready for review July 27, 2023 15:24
@jpfairbanks jpfairbanks merged commit 169876c into main Jul 27, 2023
9 checks passed
@jpfairbanks jpfairbanks deleted the llm/docs_fixes branch July 27, 2023 15:25
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.

Amend Docs
2 participants