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

Updating cookbook recipes and notebooks to reflect yt4 changes #3284

Merged
merged 60 commits into from
May 22, 2021

Conversation

chummels
Copy link
Member

This is a pass at updating the docs to reflect the current state of code in yt4. Namely, this updates the cookbook, fixing some breakages in the notebooks, and ensuring full tuples are used to specify each field reference, among some other changes.

Note, I'm removing the description surrounding n_ref in the Gadget loading data page, since it is no longer relevant in yt4: http://yt-project.org/docs/dev/examining/loading_data.html#indexing-criteria

@chummels
Copy link
Member Author

In response to the concerns about adding parentheses on the various tuples: I know it's not necessary and that the code will function correctly either way. I just thought it was cleaner to consistently specify tuples with parens surrounding them. But I can remove these if people have strong feelings about it.

@chummels
Copy link
Member Author

I'm reviewing this -- thank you for the big effort to update this. One thing I do want to flag for further thought is whether we're doing a service or a disservice when we always use explicit tuples, even when there is no ambiguity.

It was my understanding that we were meant to be using the full tuple specification as much as possible to avoid possible field clashes.

@matthewturk
Copy link
Member

@chummels I'm not saying that it's wrong; I was flagging that I am not sure that I fully support having it everywhere, especially where there aren't conflicts or ambiguity. I apologize for the unclear message that I sent -- I was not asking for any work to be done.

@chummels
Copy link
Member Author

No, I get it. It's a valid concern. I just thought we were moving to full-tuple specification everywhere. We can talk about it today in the meeting.

@neutrinoceros
Copy link
Member

I just thought it was cleaner to consistently specify tuples with parens surrounding them. But I can remove these if people have strong feelings about it.

Consistency > all

That said, here's a surprise argument in favour of leaving parens out, and encouraging the paren-free syntax:
ipython/ipython#12395

Last year, @cphyc contributed this patch to IPython (and thus, to JupyterLab and Notebook), which allows auto-completion to work on the second member of a two-tuple key in an accessor, while it previously used to work only on the first one (which wasn't really useful to us since it would then only auto-complete field types and never field names). The feature isn't released yet, and it's targeted for IPython 8.0, which is a big deal, hence the delay.

chummels and others added 5 commits May 20, 2021 13:39
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
@jzuhone
Copy link
Contributor

jzuhone commented May 21, 2021

@neutrinoceros let me know if you are going to review again or not--if not, I will merge.

@neutrinoceros
Copy link
Member

Oh sorry I forgot about this, I'll give it a go now

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

here are more suggestions in line with my first batch. I think I got all of them now.

doc/source/cookbook/fits_radio_cubes.ipynb Outdated Show resolved Hide resolved
doc/source/cookbook/fits_radio_cubes.ipynb Outdated Show resolved Hide resolved
doc/source/cookbook/fits_xray_images.ipynb Outdated Show resolved Hide resolved
doc/source/cookbook/fits_xray_images.ipynb Outdated Show resolved Hide resolved
doc/source/cookbook/fits_xray_images.ipynb Outdated Show resolved Hide resolved
doc/source/cookbook/particle_filter.py Outdated Show resolved Hide resolved
doc/source/cookbook/particle_filter.py Outdated Show resolved Hide resolved
doc/source/cookbook/particle_filter_sfr.py Outdated Show resolved Hide resolved
doc/source/cookbook/particle_filter_sfr.py Outdated Show resolved Hide resolved
doc/source/cookbook/tipsy_and_yt.ipynb Outdated Show resolved Hide resolved
chummels and others added 11 commits May 21, 2021 13:16
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
@neutrinoceros neutrinoceros merged commit c2b62be into yt-project:main May 22, 2021
@chummels chummels deleted the yt4docfix branch August 25, 2022 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs yt-4.0 feature targeted for the yt-4.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants