First steps tutorial review procesing

ArseniyKholod commented 3 months ago

This PR addresses the following review for first_steps conducted by Manuel Torrilhon.

First tutorial
[x] > * clarify that julia will actually be downloaded/installed upon first call
[x] > * BTW: on Windows you need a MS Store Account for the installation
[x] > * short comment on what happens: package installation, downloading, pre-compilation, etc
[x] > * you should give a hint that this takes time (took me 30min on a slow machine)
[x] > * BTW: Windows firewall asks for permission when installing packages
[x] > * give some details on Euler equations and 'weak blast wave' as the initial condition, like what is actually calculated?
[x] > * just quickly mention what grid, what method is used, what boundary conditions? Maybe refer for details to the next page 'Create first...'.
[ ] > * the output display is somewhat un-motivated/un-commented. It actually would make sense to suppress the output and only mention that the command will generate a lengthy output which will be discussed on the next page 'Create first...'
[ ] > * Is it possible to collapse the output cell? Would increase the readability of the tutorial.
[x] > * BTW: for me the measurement table at the end shows a different order...
[x] > * doing a plot is nice, but please mention what is shown there, like density, velocity, what time, just mention that there are waves, etc... so that the plot appears not so sterile.
[x] > * actually, some basic background about the variable 'sol' would be nice. When I call up 'sol' in Julia it does not look like the complete solution. Maybe this goes to the next tutorial (see below).
[x] > * The Euler equations from the section 'Modifying existing setup' could go up to 'Running simulation'. I understand that this all should be simple, but it is not a race about the fastest way to produce colorful fluid dynamics... Once you mention Euler Eqns in the beginning they can also be shown right away, of course clarified as example. A first time user is interested in what is (can be) calculated!
[x] > * BTW: it seems that the 'Literate.jl' package automatically includes output of code-cells, unless it is suppressed explicity? Anyway, the output cell after 'initial_condition' looks strange and not necessarily helpful.
Second tutorial
[x] > * the 'create first setup' page is generally excellent and extremely helpful!
[x] > * BTW: I think better language would be "Create your first setup", or "Create a first setup", though...
[x] > * It maybe better to suggest to the reader to type all the comments directly into Julia (this works as well, isn't it?) and only list at the end all the commands in one file (possibly only the download link in the repository). This also would give the output cells shown in the text an elevated meaning.
[x] > * maybe we can rename the advected variable. 'scalar' is very confusing, because everybody will interpret it as type-specification.
[x] > * in the spirit of the above comment, the output of 'SemidiscretizationHyperbolic()' and 'ode=semidiscretize()' should be shown (and the semi-colon removed) for complete display.
[x] > * It would make sense to quickly mention what are the default choices of fluxes, etc for DGSEM. It is not directly mentioned in the linked page 'Intro to DG', either.
[x] > * please clarify for 'SummaryCallback' what it means to 'reset timers'?
[x] > * also the statement 'if the returned callback is executed directly' is difficult to understand. Later comes the example, maybe the statement can be dropped here.
[x] > * BTW: there is also an 'AliveCallback' as far as I remember. Maybe this is nice to mention here? I am sure many people like the feel of the code working...
[x] > * BTW: Why is the output to 'CallbackSet()' so long?
[x] > * I would strongly suggest to use a simpler ODE-integrator. You started nicely with basic advection and basic discretizations, most people would expect RK3-SSP. Then the CFL also gets a meaning.
[x] > * again the visualization is nice, but it is important to explain the returned data of the computation. What exactly is stored in 'sol'
[x] > * 'plot(sol)' has been used before. It is didactically more valuable to use 'pd = PlotData2D(sol)' here right away and then clarify its usage like 'plot(pd["scalar"])'.
[x] > * the statement 'only for solutions in 2D and 3D spatial domains' sounds like this covers everything. What is missing?
[x] > * again some basic information about what is stored in the vtu-files should be given.
Third tutorial
[x] > * Short and largely fine...
[x] > * In Github Desktop, in the open-repository window (Ctrl-Shift-O) you also have to choose the tab 'URL' before you can enter 'trixi-framework/Trixi.jl'.
[x] > * Maybe the procedure to fork Trixi should be mentioned/discussed/explained, in order to encourage contributions. Pull-requests are related, but there could also be a link to where this is explained.

github-actions[bot] commented 3 months ago

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

[ ] The PR has a single goal that is clear from the PR title and/or description.
[ ] All code changes represent a single set of modifications that logically belong together.
[ ] No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

[ ] The code can be understood easily.
[ ] Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
[ ] There are no redundancies that can be removed by simple modularization/refactoring.
[ ] There are no leftover debug statements or commented code sections.
[ ] The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

[ ] New functions and types are documented with a docstring or top-level comment.
[ ] Relevant publications are referenced in docstrings (see example for formatting).
[ ] Inline comments are used to document longer or unusual code sections.
[ ] Comments describe intent ("why?") and not just functionality ("what?").
[ ] If the PR introduces a significant change or new feature, it is documented in NEWS.md.

Testing

[ ] The PR passes all tests.
[ ] New or modified lines of code are covered by tests.
[ ] New or modified tests run in less then 10 seconds.

Performance

[ ] There are no type instabilities or memory allocations in performance-critical parts.
[ ] If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

[ ] The correctness of the code was verified using appropriate tests.
[ ] If new equations/methods are added, a convergence test has been run and the results are posted in the PR.

Created with :heart: by the Trixi.jl community.

ArseniyKholod commented 3 months ago

I have addressed all suggested changes except for two unchecked points, which pertain to suppressing the trixi_include output from the first part of first steps. I believe it is helpful to see the output from Trixi's elixirs to understand what to expect. But perhaps I am mistaken.

JoshuaLampert commented 3 months ago

Just a small note: To reduce noise, you can use "Add suggestion to batch" from the GitHub interface and then only have one commit where multiple suggestions from the code review are applied.

ArseniyKholod commented 3 months ago

Just a small note: To reduce noise, you can use "Add suggestion to batch" from the GitHub interface and then only have one commit where multiple suggestions from the code review are applied.

Oh, thank you! I'll make sure to follow this next time.

DanielDoehring commented 3 months ago

Oh, thank you! I'll make sure to follow this next time.

For some reason, this is (for me) only available in the "Files changed" tab, i.e., not in the "Conversation" tab - I missed this also completely in my first PRs.

JoshuaLampert commented 3 months ago

I guess a review from @torrilhon would be nice.

torrilhon commented 3 months ago

Will do! Nice work, but I will add some minor suggestions. Give me some time, though...

ArseniyKholod commented 2 months ago

Thank you very much for your meaningful review, @torrilhon!

I applied all the suggestions I could.

sloede commented 2 months ago

Thanks a lot for your suggestions and effort @torrilhon. Would you like to perform a final review?

@ArseniyKholod Thanks a lot for your work on this. It would be great if you could modify the TODO list above and explain with a short sentence why some points will not be fixed.

ArseniyKholod commented 2 months ago

Thanks a lot for your suggestions and effort @torrilhon. Would you like to perform a final review?

@ArseniyKholod Thanks a lot for your work on this. It would be great if you could modify the TODO list above and explain with a short sentence why some points will not be fixed.

The only thing, that was not done, is suppressing outputs from code blocks of the getting_started section, e.g. from trixi_include. The reason for suppressing is that output of e.g. trixi_include was explained first time only in the next section create_first_setup. But I think it's very useful to see a real output in tutorial, as it could be an unexpected output in case a newer will run code himself. IMHO an output mostly is self-explanatory. I could be wrong... What would you say, Michael?

PS. Suppressing output of trixi_include seems to be tricky, as ; will not work here, I only could imagine to redirect the output stream somehow in a file or smth like that.

sloede commented 2 months ago

What would you say, Michael?

I'm ok either way - since you're the maintainer, I leave the decision up to you.

I don't know if this is an option from technical point of view, but what about showing an example output only (i.e., with the timespan reduced such that only ~10 steps are needed) but then suppress the original, long output?

Again, I don't know if this really improves matters, thus I'll leave it up to you

ArseniyKholod commented 2 months ago

What would you say, Michael?

I'm ok either way - since you're the maintainer, I leave the decision up to you.

I don't know if this is an option from technical point of view, but what about showing an example output only (i.e., with the timespan reduced such that only ~10 steps are needed) but then suppress the original, long output?

Again, I don't know if this really improves matters, thus I'll leave it up to you

Thanks for the suggestion! The example runs only 37 steps, so 10 steps sound like not really big reducing, since most of the output in this case is show of different structures and independent on number of steps...

sloede commented 2 months ago

What would you say, Michael?

I'm ok either way - since you're the maintainer, I leave the decision up to you. I don't know if this is an option from technical point of view, but what about showing an example output only (i.e., with the timespan reduced such that only ~10 steps are needed) but then suppress the original, long output? Again, I don't know if this really improves matters, thus I'll leave it up to you

Thanks for the suggestion! The example runs only 37 steps, so 10 steps sound like not really big reducing, since most of the output in this case is show of different structures and independent on number of steps...

OK, then maybe it is as good as it gets :-)

torrilhon commented 2 months ago

I am also fine with showing the output.

sloede commented 2 months ago

Not sure what else I need to "review" here, but I just submit another "approve"...

Thanks a lot for your feedback. I'll do a final review and then we can merge this.

ArseniyKholod commented 2 months ago

A number of small suggestions for clarity, but I'll leave it up to you if you want to include or reject them. Please ping me once your done such that we can merge this.

Thank you very much for the review, @sloede ! I committed all suggested changes.

trixi-framework / Trixi.jl