Skip to content

Edits to "Xarray and Dask"#177

Merged
scottyhq merged 8 commits intoxarray-contrib:mainfrom
yutik-nn:first_edit
Jun 15, 2023
Merged

Edits to "Xarray and Dask"#177
scottyhq merged 8 commits intoxarray-contrib:mainfrom
yutik-nn:first_edit

Conversation

@yutik-nn
Copy link
Copy Markdown
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
Copy Markdown

review-notebook-app bot commented Jun 12, 2023

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
Copy Markdown
Contributor

scottyhq commented Jun 12, 2023

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
Copy Markdown
Contributor Author

yutik-nn commented Jun 12, 2023

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
Copy Markdown
Contributor

dcherian commented Jun 14, 2023

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
Copy Markdown
Contributor

dcherian commented Jun 15, 2023

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

@yutik-nn
Copy link
Copy Markdown
Contributor Author

yutik-nn commented Jun 15, 2023

@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
Copy Markdown
Contributor

dcherian commented Jun 15, 2023

Nice work! 🥳

@JessicaS11
Copy link
Copy Markdown
Contributor

JessicaS11 commented Jun 21, 2023

@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
Copy Markdown
Contributor

scottyhq commented Jun 21, 2023

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