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] paper suggestions #137

Open
jakelangham opened this issue Sep 30, 2024 · 4 comments
Open

[JOSS] paper suggestions #137

jakelangham opened this issue Sep 30, 2024 · 4 comments

Comments

@jakelangham
Copy link

This issue relates to ongoing reviews at openjournals/joss-reviews#7018.

I have read through the draft manuscript. Overall I thought it was well written and fits the style of a JOSS paper. In addition to a few minor things included in this pr #136, I have a few suggestions listed below

  • A reference or two for the 'sparse grid combination technique' would be useful early on.
  • In the summary you say 'DisCoTec demonstrates its superiority' without making it clear what you are comparing it with. Moreover, does 'superiority' refer to accuracy, resource use, efficiency, etc?
  • Acronyms (such as HPC, UQ etc) should be defined before use.
  • The 'Statement of need' would benefit from references that cover the curse of dimensionality and the approaches that attempt to deal with it.
  • When you say 'multi-scale methods ... provide an alternative approach to address the curse of dimensionality' - if you can do so compactly, it would be good to explain how they achieve this.
  • When you say 'some other implementations of the sparse grid combination technique are available' - references should be provided if possible.
  • The second paragraph of the 'Statement of need' largely repeats material from the 'Summary' and could (in my opinion) be deleted.
  • I mentioned this already in [JOSS] Fig 1 #134, but care needs to be taken to define all mathematical notation. For example, in the combination formula, $f^{(s)}$, $f_{\vec{\ell}}$ and $c_{\vec{\ell}}$ are defined, but not $\vec{\ell}$ or $\mathcal{I}$.
  • This doesn't necessarily need to go in the paper, but could you explain how using OpenMP decreases the memory footprint?
  • In a couple of places, you provide links to github repositories that could (and perhaps should) be made into full references in the bibliography.
  • When mentioning spack - you could indicate that the purpose of this is to simplify installation on HPC systems.
  • On line 82: 'The C++ code SG++ allows to...' - allows what/who to?
@freifrauvonbleifrei
Copy link
Contributor

freifrauvonbleifrei commented Dec 6, 2024

This doesn't necessarily need to go in the paper, but could you explain how using OpenMP decreases the memory footprint?

Sure: At the core counts we are targeting, it gets impractical to use one MPI rank for every core. This is because each rank needs to store some information about addressing every other rank. Trading MPI ranks for OpenMP threads helps here.
Probably the reference on saving memory can go? I was still quite proud about it when we worked on the paper draft, but I think your question shows that it brings more confusion than clarity.

@jakelangham
Copy link
Author

Interesting. Up to you if you want to keep it in, but if you do, then I think there is room to add a brief explanation without disrupting the flow of the text too much.

freifrauvonbleifrei pushed a commit that referenced this issue Dec 10, 2024
define I, software -> references
cf. #137
@freifrauvonbleifrei
Copy link
Contributor

hi @jakelangham , I decided on a "lightweight" modification on the OpenMP / memory saving topic, but I am not sure about how much easier it really is to get the point:

https://github.com/SGpp/DisCoTec/actions/runs/12250144549/artifacts/2297939731

The second paragraph in "statement of need" however is reduced to one sentence and reads much smoother now :) thx for the good suggestion!

@jakelangham
Copy link
Author

Hi @freifrauvonbleifrei - looking good. Only a few minor gripes / typos left to clear up from my perspective:

  • As in the new documentation, I think \ell_{\max} should actually be \ell^{\max} to be consistent with the notation used for elements of these vectors + they need a backslash to render the max properly. (This is in the Fig 1 caption.)
  • Some of the new references are missing spaces between the last word and the opening (.
  • On line 76 you don't need to redefine High Performance Computing since it was already taken care of earlier. ;)
  • I would suggest consulting the editor on how best to do github citations. These can be a bit hard because of determining attribution etc (though some repositories will have details on how to cite). Maybe there is a preferred way to do this in JOSS.

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

2 participants