Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #387
JOSS paper #387
Changes from 20 commits
3e4167e
10a2701
de7c467
bb4b899
9bfea3d
c526171
fd58298
2c59a8b
cd11fd4
4049518
553bcc6
0ebc3aa
e6d56b6
7ef34ce
c6e659a
41cc8ae
e954def
2f61224
e9ac229
8e27e26
ce36c82
66b7b88
afe4feb
1e636df
dc0f903
ba2244c
453a8c1
78d3a46
783522b
8c99caf
6bf1982
9857a80
794ae95
559ca64
61f4850
fc75789
02fe3a0
8615235
f2b6f7d
40887ef
039d756
9000d60
707053b
5e57d9e
30b51ea
2d599b5
767b785
9bde536
71a9355
4ede4da
9fe4f94
7cb917c
4d7702f
f083a9a
2ef9b0f
84239c8
66cf01c
aedac51
465edc7
fd03c79
a8235e1
752e001
5d9c304
c63b33b
19168b6
bcb8df9
5672dd8
684eb5a
d9004fe
5a4e5a4
d4e5125
b1c1062
e81500e
f7e864e
f09bab1
aa93655
eee6555
75e72ce
0018b19
67eddb7
579c490
04a9378
4dc8fa9
81eedda
3e863bc
65c3259
d76df12
a88f020
58d582b
f40d6ce
99e1e26
cc28a4a
f54e7d4
9813e99
000c992
a71fa34
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above, what is the need here to reword the entire paragraph? Especially because you introduced some wordings that aren't as concise as before. "grid-to-spectral space interpolations", a spectral transform is strictly speaking not an interpolation. We do have interpolations between various RingGrids, but they are distinct. "spherical space" isn't a commonly used term, neither does it include the notion of spectral that "spectral space" would have or "spherical harmonic space". Please try to avoid introducing these inconcise terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my review comment from above. Why did you rewrite the entire paragraph? If there's need for it I'd be happy to accept your suggestions, but I cannot see what was wrong with the previous version. "draws inspiration from" is less correct than the previous "heavily influenced"; I don't know what "library-style approach" is, but the previous "library-style interface" is clearer as it refers to the model being used as a software library, which various functions that can be interactively combined. Also "models are built from ground up" is not correct, as it's not the ground where one has to start from, it's the library level that we provide. "bottom-up" is more relative and a known term in constrast to "top-down". "the model object produces a simulation" is also not correct, there's a reason why I wrote "initialize" before. And you removed the important point of "interactivity
far beyond a monolithic interface", that should remain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful, garbage in, garbage out. What is sensible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial conditions for the shallow water model and the primitive equations by default are the Galewsky et al, 2004 and the Jablonowksi and Williamson 2006 test cases, respectively. "sensible" means that inexperienced users are not confronted with simulations that blow up by default etc. So I don't know what your concern is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added that defaults are well-established test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my comments from above. Please justify why there is a need to reword the entire paragraph, especially because you introduced several points that are less correct, less concise as they were before. You write "distinct type", which I'd like to avoid because these types are sub and supertypes of each other, so they have a relation and depending on the type tree aren't always distinguished and can be subtyped. You write "functions need modification for this type" no, that's not how multiple dispatch works. There's a reason I wrote "extend" rather than "modify" because with multiple dispatch you don't have to modify or overwrite the previous definition of a function, rather you extend it by defining a new method, that's very different and in fact our entire approach wouldn't work if it wasn't for that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar as before, I don't see the win over the new paragraph compared to the old, especially because we now have two very long sentences, whereas the previous version had a clear summary sentence to start with before going into the details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tautology
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no. SpeedyWeather could be number format flexible without its transform and vice versa. I've been playing with the idea to use one format for the dynamical core exclusive the transform, but always Float32 for the transform as FFTW only supports Float32/64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what @dmey means here is that 32-bit implies single precision and vice versa, hence stating both is tautological.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay. Yeah, sure happy to write just
single precision
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you had problem with half precision etc. Is this actually working? Else rephrase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do I say this is running in half precision? The FFT will switch between FFTW and GenericFFT depending on the format, using half precision is a precision issue not a compiler issue anymore. Other formats like posits may require certain methods that I haven't defined yet, but that's not a problem of SpeedyWeather.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still an extra
and
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the first sentences you wrote and will incorporate them in the next days. Can you clarify whether there's a difference between your paper and your thesis, or whether for the sake of this citation they actually refer to the same piece of work. Very happy to cite your paper!