-
Notifications
You must be signed in to change notification settings - Fork 111
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
improving the advanced indexing notebooks #264
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
I am not sure why the preview of notebook is failing. There is a duplicate label warning :
Nevermind this was caused by duplicated exercise names. |
👏🏾 👏 i love this image! we should get it in the main docs too. |
♻️ PR Preview 383eeae has been successfully destroyed since this PR has been closed. 🤖 By surge-preview |
@negin513 just a heads up - I added a couple commits to sync with the latest environment and enable the website preview link, so you can see your changes here https://xarray-contrib-xarray-tutorial-preview-pr-264.surge.sh/intermediate/indexing/advanced-indexing.html |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@dcherian and @JessicaS11 Thanks so much for your review and feedback provided. I have addressed all the comments and implemented your suggestions. Can you please take a look at this and let me know if this looks good for merging? Thanks! |
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.
This is a wonderful improvement @negin513 thank you! I love the image and narrative flows really well. Feel free to merge. two small ideas:
- The plot will look better on the website if you add
%config InlineBackend.figure_format='retina'
in the top cell - Not sure if it's worth adding a sentence or linking to np.ix_ to achieve the orthogonal result in numpy
np_array[np.ix_([0, 2, 4], [0, 2, 4])]
If someone knows why this is named so obscurely (.ix_) I'd love to know :)
I think meshgrid is an alternative to ix_. I won't have time to review but thanks Negin! |
for more information, see https://pre-commit.ci
Thanks @scottyhq , @dcherian , and @JessicaS11 for great comments. I have added the following:
|
for more information, see https://pre-commit.ci
This PR should address issue #206 .
I created and added a schematic image to explain different indexing types. I also added more examples to the notebook for more clarify.