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

[JOSS] Functionality documentation #129

Open
EmilyBourne opened this issue Aug 19, 2024 · 12 comments
Open

[JOSS] Functionality documentation #129

EmilyBourne opened this issue Aug 19, 2024 · 12 comments
Assignees

Comments

@EmilyBourne
Copy link

Regarding openjournals/joss-reviews#7018 - " Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?"

I cannot find documentation beyond the READMEs:

The webpage https://sparsegrids.org/ is mentioned in the "About" section but does not seem to mention DisCoTec:
image

Is there API documentation describing the available classes/methods somewhere? Perhaps something generated with doxygen?

@pfluegdk
Copy link
Member

@EmilyBourne Thanks for pointing this out. DisCoTec was indeed missing in the list of software. This does not solve the API documentation issue, but I added DisCoTec on sparsegrids.org.

@freifrauvonbleifrei
Copy link
Contributor

@obersteiner , @vancraar and me are currently putting the finishing touches on the new documentation at https://discotec.readthedocs.io/ !

At the same time, the main README is getting significantly shorter, see here: https://github.com/SGpp/DisCoTec/tree/feature_documentation . But it now also tells you "When to (not) use DisCoTec" :)

@EmilyBourne @jakelangham we'll continue hacking on it for a few more days and let you know when we think we are "done" -- nonetheless, feel free to leave any intermediate feedback!

@freifrauvonbleifrei freifrauvonbleifrei self-assigned this Sep 18, 2024
@freifrauvonbleifrei
Copy link
Contributor

@EmilyBourne @jakelangham I believe the documentation is fine for review! -> https://discotec.readthedocs.io/

with a few edits pending, which can be followed in #133

Please do let us know if things are unclear, your "outside" perspective can really help us make this a great documentation :)

@jakelangham
Copy link

Further to @EmilyBourne's initial comment - there should be a link to the new docs somewhere in the README.md that's at least as prominent as the sparsegrids webpage link.

@jakelangham
Copy link

Also to address this bullet point in the JOSS review: 'Do the authors clearly state what problems the software is designed to solve and who the target audience is?' I think you could be a little clearer, either in the docs or the README.

It is up to you how you do this but one suggestion would be to add some more text to the 'What is Discotec?' section that explains/advertises the benefits of using it and perhaps suggests the sorts of applications it might be particular suited for.

@freifrauvonbleifrei
Copy link
Contributor

Further to @EmilyBourne's initial comment - there should be a link to the new docs somewhere in the README.md that's at least as prominent as the sparsegrids webpage link.

I am trying something -- can you spot it?

@freifrauvonbleifrei
Copy link
Contributor

It is up to you how you do this but one suggestion would be to add some more text to the 'What is Discotec?' section that explains/advertises the benefits of using it and perhaps suggests the sorts of applications it might be particular suited for.

@jakelangham I thought that the "When (not) to use DisCoTec" sections could fulfill this function, but if you didn't find it, it's obviously not prominent enough yet (or not what you were looking for?). Maybe it would be more logical to put above the installation section anyways...

@jakelangham
Copy link

@freifrauvonbleifrei Ah, I had not fully appreciated yesterday that I should be reviewing the feature_documentation branch... What you've written looks ideal and just what I was looking for. It could go a bit higher up in the README but that's up to you.

p.s. It might be a requirement for JOSS that these commits are merged into main before publication.

freifrauvonbleifrei added a commit that referenced this issue Sep 27, 2024
@jakelangham
Copy link

@freifrauvonbleifrei I've finished reading through the documentation. The effort to produce this is greatly appreciated, but I think it could be improved in places, so here are my comments - a mixture of minor things and broader ideas that I think would help. Thanks.

  • In 'Run the tests', perhaps you could indicate the resources they will use, just in case anyone is like me and daft enough to try running them on their desktop first!
  • In 'Run an Example' I think it would be useful to take the space to demonstrate running an instructive example and analysing its output. As a new user I can see that there are many examples which would help me to get started, but these are not self-documenting and in many cases it is not clear what it is that the code is supposed to do. Therefore, more broadly, at least documenting the high level purpose of each example would be valuable, so that a motivated user can read the code from an informed starting point. In some cases you could point the user to the other READMEs that exist elsewhere in the repository.
  • In 'Paralelism in DisCoTec', I am a little unclear on what the 'process groups' actually do and how this relates to the different grids that are in use. The impression I got was that each process group is responsible for a particular grid (or perhaps a collection of grids?) and when these need to be combined, the process groups do communication with each other. Then there is parallelism within process groups which I presume may be delegated to the underlying solver code(?). However, this isn't made explicit, so I suspect I could be wrong on this. Furthermore, the later figure indicates that one is free to choose how many process groups there are, but does this have to e.g. be a number that evenly divides the number of grids?
  • In the second figure of 'Paralelism in DisCoTec', it might be nice to indicate the underlying problem that was being solved up front. You refer to 'the advection solver step' but that context needs to be set up in advance.
  • I was just about able to follow the tutorial, which I think is very valuable. Nevertheless, to understand the code snippets I often wanted to see more context on what some of the functions called are doing. This led me to click on the 'Library API', which is yet to exist. I hate to say it, but this could be useful. API documentation is listed as a requirement in the JOSS review criteria (https://joss.readthedocs.io/en/latest/review_criteria.html#api-documentation) and seems particularly important for a code such as this which is designed to work like a library that users build codes on top of.
  • I got slightly confused in the tutorial how many dimensions the example code uses due to the conventions used in num_points, which change between the initial pseudocode and the actual code further down. (Also when my brain sees x, y, vx, vy, it thinks space = (x,y) and velocity = (vx, vy)...) Consider communicating this up front.
  • In the same section, you refer to the 'cartesian process vector'. I don't think this is defined in the tutorial or elsewhere. Also I suppose Cartesian should be capitalised.
  • 'Advanced topics' relies heavily on references to journal publications that are not open access. I think it is ok to include these as long as they are just additional information and not required to understand the software (since the documentation itself must be openly available). Nevertheless, I thought I should raise this so you can check whether things would make sense for someone who couldn't access those papers. For example, at one point you refer to 'relatively laminar simulations', but this can only be understood by someone that has enough of your paper to know that it concerns simulations of turbulent plasmas.

@freifrauvonbleifrei
Copy link
Contributor

In the second figure of 'Paralelism in DisCoTec', it might be nice to indicate the underlying problem that was being solved up front. You refer to 'the advection solver step' but that context needs to be set up in advance.

I feel some inner resistance to actually introduce the solver here, because I think it would distract from the main point I am trying to make. What is your opinion on replacing "advection solver" with "memory-bandwidth-bound 6-D PDE solver" (or similar) in this section @jakelangham ?

freifrauvonbleifrei pushed a commit that referenced this issue Dec 6, 2024
(addresses 6th point raised by @jakelangham in issue #129)
freifrauvonbleifrei pushed a commit that referenced this issue Dec 6, 2024
(addresses last of @jakelangham's points in issue #129 )
@freifrauvonbleifrei
Copy link
Contributor

@jakelangham IMHO I have addressed the issues you raised in the current version of PR #139, see here https://discotec--139.org.readthedocs.build/en/139/ (minus the "advection solver", commented above). Let me know in case there is more open for you! (also to @EmilyBourne of course!)

@jakelangham
Copy link

Hi @freifrauvonbleifrei - thanks for this. I think you really made some great improvements here. I've read through and overall things are much clearer. In particular the considerable work on the API is greatly appreciated. Only 2 tiny comments:

  • In 'Parallelism in DisCoTec', there is an \ell whose LaTeX is not rendering.
  • Looking back at my comment around the 'advection solver' line, I don't think I was asking to define the PDE per se. Perhaps it is just a subtle English language thing (or just my subjective feeling), but when you say 'the advection PDE solver', it makes me think I must have missed where you told me about this particular PDE previously. So perhaps you could instead say something like 'We see the timings (in seconds) for the solver and combination steps respectively, for an illustrative (6-D) PDE problem.'

freifrauvonbleifrei pushed a commit that referenced this issue Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants