-
Notifications
You must be signed in to change notification settings - Fork 494
Improved "AoA vs CAS.ipynb" example #1290
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
base: master
Are you sure you want to change the base?
Conversation
@agodemar you don't need to open a new PR when you are making changes to a PR. Just update the code in your branch ( |
Yep, and it doesn't matter if there a bunch of commits to get things right, we'll do a squash merge so all the individual commits won't be visible in JSBSim's master branch once we merge it in. In terms of your Matplotlib figures not originally being included and visible in the Jupyter notebook it looked like you hadn't executed the notebook before the commit since there was no execution count for the code cells. The best option to ensure that you have all the required outputs in the notebook and also to ensure that the output of each code cell actually matches the code is to perform a "Restart Kernel and run all cells" before each commit.
The static PNG images aren't embedded in the notebook, the notebook has references to the PNG images in the
If an image is actually embedded in the notebook then you will see the image data base64 encoded inside the .ipynb file, e.g. like the following for the Matplotlib output images from the code cells.
For the static PNG images there are some pros and cons in terms of having them actually embedded in the .ipynb file versus being links to external image files. If they are embedded then you can pass around/email a single .ipynb file to someone. But if multiple notebooks may use some of the same static image files then it might be better to have them all reference a single external image file, the ipynb file is smaller etc. |
Given we have a lot more context up-front now I think we should then also have a brief explanation section after the output graphs to explain why the graphs look the way they do, in particular to do with the graph's range For example, maybe something along these lines: For a given configuration In fact, looking at the current graphs, it's not immediately clear to me what is limiting |
Yes, I agree with your observations. They demonstrate what is known to aerospace engineers from Flight Mechanics courses, in particular from basic concepts of level flight performance. Also, these notebooks should provide to the general audience some practical examples of how a fairly advanced flight simulation tool (such as JSBSim coupled with Python & Jupyter) is able to reproduce annotated engineering simulations with a reasonable effort. I will keep working on refining this first example. |
I've always been a bit suspicious of the outputs for this example for the +5% cg cases, but have never taken a closer look. Given we're updating the example I'll take a closer look to try and understand what is happening with the +5% cg cases and how realistic the results are. |
The |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1290 +/- ##
=======================================
Coverage 24.76% 24.76%
=======================================
Files 169 169
Lines 19598 19598
=======================================
Hits 4854 4854
Misses 14744 14744 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Updated "AoA vs CAS.ipynb" example. PNG images embedded inside the Jupyter notebook.
Added more context at the beginning of the notebook.
Added more figures taken from JSBSim online reference manual.
Made a first plot of trimmed states with one weight and one x-cg.
Improved the original plot.