Skip to content

Edits to "Xarray and Dask" #177

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jun 15, 2023
Merged

Edits to "Xarray and Dask" #177

merged 8 commits into from
Jun 15, 2023

Conversation

yutik-nn
Copy link
Contributor

@yutik-nn yutik-nn commented Jun 12, 2023 •
edited
Loading

changes minor points that might be causing confusion to the user (1. variable misnames 2. specifying the directory in the cluster was misleading )

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@yutik-nn yutik-nn marked this pull request as ready for review June 12, 2023 17:30
@yutik-nn yutik-nn mentioned this pull request Jun 12, 2023
Open
@scottyhq
Copy link
Contributor

great catch with the mismatched names in the exercise @yutik-nn !

  1. specifying the directory in the cluster was misleading

I agree that client = Client(local_directory='/tmp') is unclear, it makes it seem like we might have data in that folder.

The reason for that setting was discussed here, and it's also good practice to use 'scratch spaces' for temporary files
#75 (comment) ,

Instead of removing local_directory='/tmp' what do you think of adding a sentence above saying something like "dask uses the current working directory for writing temporary files by default. We use the argument local_directory='/tmp' below to instead use a temporary scratch folder" ?

@yutik-nn
Copy link
Contributor Author

Thank you for your comments @scottyhq !
The reasoning behind me removing local_directory='/tmp' altogether was that it appears in many other tutorials so it seemed like an unnecessarily complicated thing to have a specified folder here, but I see your reasoning now.
Yes, I think adding a sentence explaining it is a good suggestion!

@dcherian
Copy link
Contributor

Yes, I think adding a sentence explaining it is a good suggestion!

Ah yes, let's add that and then merge.

@dcherian dcherian changed the title First edit Edits to "Xarray and Dask" Jun 14, 2023
@dcherian
Copy link
Contributor

@yutik-nn did we decide that adding info on persist was a distraction?

@yutik-nn
Copy link
Contributor Author

@yutik-nn did we decide that adding info on persist was a distraction?

I did not do anything to the section yet, honestly. It seems to have appeared in edits because I removed some characters? But I did not touch it consciously. I added the sentence about the scratch directory only.

@scottyhq scottyhq merged commit 05152be into xarray-contrib:main Jun 15, 2023
@dcherian
Copy link
Contributor

Nice work! 🥳

@JessicaS11
Copy link
Contributor

@yutik-nn @scottyhq @dcherian Based on this discussion, shouldn't the line of code have stayed as client = Client(local_directory='/tmp') rather than client = Client()?

@scottyhq
Copy link
Contributor

whoops, i missed that! @yutik-nn would you mind a follow-up PR to add that back in?

dcherian added a commit to dcherian/xarray-tutorial that referenced this pull request Jul 3, 2023
@dcherian
Merge remote-tracking branch 'upstream/main' into sidebar-update
22c0249 
* upstream/main:
  move custom accessors to advanced and add more complex, runnable examples (xarray-contrib#168)
  Update and reorganize apply_ufunc material (xarray-contrib#180)
  Add jupyterlab-myst 2.0 and sphinx-exercise (xarray-contrib#187)
  Add jupyterlab-myst (xarray-contrib#182)
  Add codespace config for scipy 2023 (xarray-contrib#184)
  Add SciPy  Instructions (xarray-contrib#183)
  Edits to "Xarray and Dask" (xarray-contrib#177)
  Setup PR Preview workflow (xarray-contrib#175)
  contents write permissions for github token (xarray-contrib#178)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottyhq scottyhq scottyhq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yutik-nn @scottyhq @dcherian @JessicaS11