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 improvements/typos (MATLAB all kernels; Fortran just Stokes) #37

Merged
merged 15 commits into from
Aug 8, 2023

Conversation

ahbarnett
Copy link
Collaborator

@ahbarnett ahbarnett commented Aug 7, 2023

This documents the MATLAB/octave interfaces better for all 4 kernels, and the Stokes fortran source code docs. It does not change the website (docs/) or any lines of code apart from one example.

Mixture of:

  • more matlab-like style
  • adding Contents file, (eg do help(pwd) from matlab directory), labeling legacy codes as such.
  • typos in math
  • clarifications of kernels, args, direct routines (removing doc duplication for future ease of maintenance). Makes easier for nontechnical user to read.
  • adding a Stokes simple test, cleaned existing Stokes example

Note: since I had to run "make mex" there are some gateway .c diffs. Let me know if problem.

To do: (not in this PR?)

  • EMFMM3D missing math for kernel sums (only given as operators)
  • the formulae for sums could be made more explicit (source vs target sums)?
  • remove legacy codes/interfaces, cleaner if not in same repo.
  • add Stokes stresslet cases (rotlet...), grad outputs. (enhancement)

Discussion: aim to reduce duplication of docs, otherwise future changes will be hard to maintain. Clearly matlab interfaces should have own docs, as we have. But fortran source duplicates the RTD website. One option is have sources point to website for main interfaces, but internal subroutines all stay documented in the source.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was generated with a slightly newer mwrap (1.1). It compiled fine on my system. @mrachh up to you to keep or revert.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same story with fmm3d_legacy.c

@askhamwhat
Copy link
Collaborator

@ahbarnett The documentation changes look great.

For your bullets above:

  • EMFMM3D missing math for kernel sums (only given as operators). ---> I say let's do in this PR if @mrachh has time.
  • the formulae for sums could be made more explicit (source vs target sums)? ---> I'll take a stab at this for this PR. It may take a little while to pull it all through...
  • remove legacy codes/interfaces, cleaner if not in same repo. ---> I'm sympathetic to this but I see potential issues. You want them tied to the FMM version that they work with (I think this could be done with careful versioning but it seems more complicated). I'd say a separate PR if it's done.
  • add Stokes stresslet cases (rotlet...), grad outputs. (enhancement) ---> definitely needed but I'd say a separate PR. I've made this issue feature: add other stresslet options #38

@askhamwhat
Copy link
Collaborator

I decided not to pull the documentation change for source vs target sums through the Fortran for now as this would be a larger scale change (and the Fortran docs are not consistent with each other currently). I agree that this should probably be made clear.

@askhamwhat
Copy link
Collaborator

@mrachh you're off the hook for the emfmm3d docs. I put in the formulae.

Copy link
Collaborator

@askhamwhat askhamwhat left a comment

Choose a reason for hiding this comment

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

This is good to go for me. @mrachh I put in the formulae for emfmm3d. I left in these newer mwrap generated files. Up to you to revert.

@mrachh
Copy link
Member

mrachh commented Aug 8, 2023

These look great guys. Thanks!

@ahbarnett: Did you pull through the docs from the .mw file or edited the .m files after generating them through mwrap.
When I regenerate them on my machine, it seems that they were later edited so that the docs are not consistently generated.

This has been fixed now.

Regarding the other todo's

  • Having source and target formulae are needed, it seems pretty evident what they are and the docs explain the quantities. As far as I know the fortran docs have different formulae in for sources and targets based on the interface. The fortran docs were structured so because of the fortran uis being separate for source-> source, source->target and source->source+target, while matlab behaves more like a guru interface and adds to confusion in the docs if only using with sources.

  • Having the legacy interfaces is also alright, if only one of the two repos remains supported in the long run, we can just point to the one that has both interfaces, it is just a few more files after all.

I've renamed some test files to be more consistent with matlab test notation, and updated contents.m accordingly

@askhamwhat
Copy link
Collaborator

@mrachh I think I messed that up (docs in the .m instead of the .mw). I can try to fix. I think Alex made the changes in the .mw...

@ahbarnett
Copy link
Collaborator Author

Yes I did change the .mw, as the SSOT. But at the very end I forgot to run make mex to propagate changes to the .m - sorry about that. Looks like you fixed that, plus other good stuff.

I might suggest that emfmm3d return U.Etarg, U.curlEtarg, U.divEtarg
for consistency. That way when source-eval is added, it won't break user code!
That should be a separate PR since it's not just a doc change.

In general, there is a duplication problem in docs that's worth fixing. Ie,
thinking about SSOT for updating local (language-facing) docs vs website (docs/*.rst) needs to be done. Eg, can the website Matlab section simply auto-paste in the .m docs?
Otherwise we'll be always spending 2-3 times longer than needed keeping multiple docs consistent and updated...
Cheers, Alex

@ahbarnett ahbarnett merged commit 3fef0a3 into flatironinstitute:master Aug 8, 2023
4 checks passed
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