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

added exercise #115

Merged
merged 6 commits into from
Apr 21, 2021
Merged

added exercise #115

merged 6 commits into from
Apr 21, 2021

Conversation

PhiSpel
Copy link
Collaborator

@PhiSpel PhiSpel commented Apr 19, 2021

closes #102

comparing explicit and implicit solution for dahlquist and prothero equations

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -0,0 +1,227 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I would remove references to the "Vorlesung". It implies that this exercise cannot be solved without the context of the lecture, while this should be a collection of exercises useful to everyone. What we can do later, is add a reference to a book chapter.


Reply via ReviewNB

@@ -0,0 +1,227 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

  • The %%file dalquist.m is not necessary here, and may lead to not intended behavior: It tells the octave kernel to save the contents starting from the second line to a file dalquist.m, instead of executing the commands. Since the file dalquist.m is not used subsequently, I suggest removing this line. BTW, I think because you have %% file instead of %%file the line is interpreted as a normal code comment. But even then: What is the benefit of this code comment?
  • So far, we agreed on English code comments.
  • Can you include the output of this function?

Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I now tried to run the Notebook and it didn't work, because clearvars is apparently not implemented in Octave, could you please make sure, that your code works with Octave?

@@ -0,0 +1,227 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

You can explicitly reference line 21 for the second bullet point, since this appears odd when first reading the code. Let the reader think about, why you would choose the step sizes as h_selection = [0.5 * 2/abs(lambda), 1.01 * 2/abs(lambda)];.


Reply via ReviewNB

@@ -0,0 +1,227 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

One g(t) is not in math mode. Can we ask the reader to verify that the given solution is in fact the exact solution?


Reply via ReviewNB

@@ -0,0 +1,227 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

See above: No, %%file, English comments, visual output.


Reply via ReviewNB

@@ -0,0 +1,227 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I don't particularly like the title "Aufgabe". It looks weird in the TOC on the right and it is a lost subsection. And there has been another Aufgabe further above.

Also, can you be more precise, as what you mean by *g(t) = 4t^2 implementieren*? You obviously don't mean ;) :

funciton y = g(t)
% implement g(t) = 4*t^2, easy:
   y = 4*t^2;

Reply via ReviewNB

@joergbrech
Copy link
Member

Sorry, I thought we get a nice visualization from ReviewNB here on Github. You can checkout my comments here: https://app.reviewnb.com/joergbrech/Modellbildung-und-Simulation/pull/115/

@PhiSpel
Copy link
Collaborator Author

PhiSpel commented Apr 19, 2021

I looked at the comments on ReviewNB. The last commit addresses all of them.

I did not find a function to directly refer changes to a comment on ReviewNB, hopefully you can trace the changes.

@joergbrech
Copy link
Member

I did not find a function to directly refer changes to a comment on ReviewNB, hopefully you can trace the changes.

Yes, no problem! I think you have to login (with Github) to NBReview to comment.

]
},
{
"cell_type": "markdown",
Copy link
Member

@joergbrech joergbrech Apr 19, 2021

Choose a reason for hiding this comment

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

I would prefer, if you use the normal jupyter-notebook output for this. Otherwise, if a user wants to run the code interactively, they will have two outputs. Also, the image link seems to be dead: https://963-168147770-gh.circle-artifacts.com/0/html/07_num_ode/02_stabilitaet_eulerverfahren.html

For this it would be nice if the images were formatted nicely

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't manage to create the jupyter-notebook output in the JupyterLab editor. Where can I find some instructions for this?

Copy link
Member

Choose a reason for hiding this comment

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

What exactly fails? Did you follow the steps from our Contribution Guide? Specifically, click on "Initial Setup" at the beginning of the file.

You need an octave installation, jupyter-notebook and the octave_kernel (which is in our requirements.txt file). Then jupyter-notebook will run the octave kernel instead of the default python3 kernel to execute the code. All plots will be rendered in the notebook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Octave was not the issue, it was nodejs missing.
I also needed to set the graphics_toolkit to "gnuplot".

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, good to know, I never encountered this problem. I wonder if we should document this somewhere? Maybe a troubleshooting section in the wiki... what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know. There actually was an according message in the console for JupyterLab, which is how I found out. "gnuplot" is also supposed to be the default graphics_toolkit, I'm not sure why it was not set as such in my kernel.

One thing which could be added is that "sudo" is a linux-specific command, so I used the Octave installation from GNU and set OCTAVE_EXECUTABLE to the correct path. The Miniconda prompt was not able to load the packages from the available channels.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Octave was not the issue, it was nodejs missing.
I also needed to set the graphics_toolkit to "gnuplot".

For future reference, the discussion about the default Octave toolkit can be found here.

"execution_count": null,
"metadata": {
"tags": [
"hide_input"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this code block hidden?

"Variieren Sie nun die Parameterauswahl in `lambda_selection` und `h_selection`.\n",
"\n",
"- Wie ändert sich die Lösung?\n",
"- Warum ist `h` in Zeile 21 abhängig von `lambda` gewählt? Was passiert, wenn `h` unabhängig von `lambda` gewählt wird? Was, wenn `h` für das explizite Verfahren nur minimal kleiner als $\\frac{2}{|\\lambda|}$ ist?\n",
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure, it's line 21 anymore. And I just realized, that the line numbers are not rendered in the html output (I saw them in NBreview), sorry. So maybe it would be best to repeat the formula for the two step sizes.

changed clearvars to clear
I still need to change the figure size for a properly visible output
@PhiSpel PhiSpel removed the request for review from rstrickstrock April 19, 2021 12:35
@joergbrech
Copy link
Member

Ok, we are getting there! Sorry for being naggy, but as a final thing, do you think we can beautify the graphical output? The graphs are a bit small and hard to read, and a legend would be nice.

@PhiSpel
Copy link
Collaborator Author

PhiSpel commented Apr 19, 2021

Ok, we are getting there! Sorry for being naggy, but as a final thing, do you think we can beautify the graphical output? The graphs are a bit small and hard to read, and a legend would be nice.

Yes, definitely! I'm on it once I'm done with my lectures for this week :) The trouble with the figure size is that the default output as a fig has quite a coarse resolution. But I will try to figure something out the next days.

@PhiSpel
Copy link
Collaborator Author

PhiSpel commented Apr 20, 2021

So, I tried to do two things:

  1. Resizing the figure. This I managed.
  2. Adding a title to the figure. This is not a function in Octave, as it is missing suptitle and sgtitle.
  3. Adding a legend. Adding a legend to a single subplot works, but hides half of the subplot, as the resizing of the figure also affects the legend. Creating an extra subplot with only the legend will not work, as the legend is always placed within the figure it refers to and is hidden once the plot is hidden, see here and here. Other options are only available in MATLAB (tiledlayout), or very laborious, by exporting and saving the legend and then reimporting it. The best I could come up with is in the last commit, with a legend below.

edit: When running on the built version, a box around the legend is left, I do not know why..

@joergbrech
Copy link
Member

It already looks much better! The only not so nice thing is that you see the dummy plot as a red x somewhere in the legend. Here is one more thing you could try, I think it should be Octave-compatible: https://stackoverflow.com/questions/41454174/how-to-have-a-common-legend-for-subplots

@PhiSpel
Copy link
Collaborator Author

PhiSpel commented Apr 20, 2021

It already looks much better! The only not so nice thing is that you see the dummy plot as a red x somewhere in the legend. Here is one more thing you could try, I think it should be Octave-compatible: https://stackoverflow.com/questions/41454174/how-to-have-a-common-legend-for-subplots

I saw this solution before, but unfortunately it does not work on Octave, neither on my local engine nor on the mybinder.org engine. The code they use an example works in MATLAB, but not on Octave, either.

@joergbrech
Copy link
Member

That's unfortunate...but ok. It is fine as it is! Let's not overengineer this

@joergbrech
Copy link
Member

Thanks for your contribution!

joergbrech
joergbrech previously approved these changes Apr 20, 2021
@joergbrech
Copy link
Member

Oh wait, can you merge/rebase on master? The TOC order is messed up again and I believe we need the changes from joergbrech/Modellbildung-und-Simulation@52bbd26 to fix it. I would like to see that the order is ok in this branch before merging this into master.

@joergbrech joergbrech dismissed their stale review April 20, 2021 15:19

We need to rebase on/merge master to make sure that this PR does not mess up the TOC order

@joergbrech
Copy link
Member

Thanks! Looks good!

@joergbrech joergbrech merged commit c624566 into master Apr 21, 2021
@joergbrech joergbrech deleted the PhilSpel-Euler branch April 21, 2021 11:11
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.

Add exercise for stability of Euler integrator
2 participants