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

Faq pull request (According to pull request #7604 & issue #1285 #7638

Merged
merged 27 commits into from
Mar 26, 2023
Merged

Faq pull request (According to pull request #7604 & issue #1285 #7638

merged 27 commits into from
Mar 26, 2023

Conversation

harshitha1201
Copy link
Contributor

@harshitha1201 harshitha1201 commented Mar 16, 2023

@TomNicholas
Please review the changes

closes part of #1285

Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Hi @harshitha1201 - thanks for working on this!

Is there are reason why you made a second pull request here rather than continuing with #7604? Generally we try to push more commits to the same branch, so as to keep a record of reviewer's feedback all in one place.

I've added a couple more suggestions anyway.

doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
@harshitha1201
Copy link
Contributor Author

@TomNicholas I have made the changes as requested, please look into them

@harshitha1201
Copy link
Contributor Author

Hi @harshitha1201 - thanks for working on this!

Is there are reason why you made a second pull request here rather than continuing with #7604? Generally we try to push more commits to the same branch, so as to keep a record of reviewer's feedback all in one place.

I've added a couple more suggestions anyway.

I was unable to commit, so I created another PR. This won't happen again.

Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Thanks @harshitha1201 ! Just a few more comments!

doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Show resolved Hide resolved
doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
@harshitha1201
Copy link
Contributor Author

@TomNicholas
I have done all changes as requested. Please review.

@headtr1ck headtr1ck added topic-documentation Outreachy-2023 Participation in Outreachy.org 2023 labels Mar 22, 2023
Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

I think this looks already good.
Just added a few comments, mainly personal opinion :)

doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved

NetCDF
------
If you are reading a netCDF file with a ".nc" extension, the default engine is `netcdf4`. However if you have files with non-standard extensions or if the file format is ambiguous. Specify the engine explicitly when reading files with xarray, to ensure that the correct backend is used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we mention the scipy and h5netcdf engines here explicitely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned scipy engine, but I'm unable to open the netcdf4 file using h5netcdf engine.

doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
@harshitha1201
Copy link
Contributor Author

@TomNicholas please review

@harshitha1201
Copy link
Contributor Author

@headtr1ck I have done the changes as requested, please review

Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

looking better and better :)

doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Outdated Show resolved Hide resolved
doc/getting-started-guide/faq.rst Show resolved Hide resolved
doc/getting-started-guide/faq.rst Show resolved Hide resolved
@harshitha1201
Copy link
Contributor Author

@headtr1ck I have done the changes, please review

@harshitha1201
Copy link
Contributor Author

@TomNicholas please review all the changes done.

Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

This is looking pretty good to me now! Well done @harshitha1201 !

If others agree I will merge it.

@headtr1ck headtr1ck added the plan to merge Final call for comments label Mar 24, 2023
@harshitha1201
Copy link
Contributor Author

This is looking pretty good to me now! Well done @harshitha1201 !

If others agree I will merge it.

Thank you!

@headtr1ck
Copy link
Collaborator

@harshitha1201 feel free to add an entry in doc/whats-new.rst.
Then we can merge this. Thanks!

@harshitha1201
Copy link
Contributor Author

Sure, can you please elaborate @headtr1ck

@headtr1ck
Copy link
Collaborator

In https://github.com/pydata/xarray/blob/main/doc/whats-new.rst

Add a line under the documentation section explaining what you have added. Use the rest of the file as a guideline on how to format.

@harshitha1201
Copy link
Contributor Author

@headtr1ck done! Thank you

doc/whats-new.rst Outdated Show resolved Hide resolved
doc/whats-new.rst Outdated Show resolved Hide resolved
@headtr1ck headtr1ck enabled auto-merge (squash) March 26, 2023 18:43
@headtr1ck
Copy link
Collaborator

Thanks for this contribution!

@harshitha1201
Copy link
Contributor Author

Thanks for this contribution!

Thank you.

@headtr1ck headtr1ck merged commit a28e9b5 into pydata:main Mar 26, 2023
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 26, 2023
* main: (40 commits)
  Faq pull request (According to pull request pydata#7604 & issue pydata#1285 (pydata#7638)
  add timeouts for tests (pydata#7657)
  Pull Request Labeler - Undo workaround sync-labels bug (pydata#7667)
  [pre-commit.ci] pre-commit autoupdate (pydata#7651)
  Allow all integer dtypes in `polyval` (pydata#7619)
  [skip-ci] dev whats-new (pydata#7660)
  Redo whats-new for 2023.03.0 (pydata#7659)
  Set copy=False when calling pd.Series (pydata#7642)
  Pin pandas < 2 (pydata#7650)
  Whats-new for release 2023.03.0 (pydata#7643)
  Bump pypa/gh-action-pypi-publish from 1.7.1 to 1.8.1 (pydata#7648)
  Use more descriptive link texts (pydata#7625)
  Fix missing 'dim' argument in _get_nan_block_lengths (pydata#7598)
  Fix `pcolormesh` with str coords (pydata#7612)
  [skip-ci] Fix groupby binary ops benchmarks (pydata#7603)
  Remove incomplete sentence in IO docs (pydata#7631)
  Allow indexing unindexed dimensions using dask arrays (pydata#5873)
  Bump pypa/gh-action-pypi-publish from 1.6.4 to 1.7.1 (pydata#7618)
  [pre-commit.ci] pre-commit autoupdate (pydata#7620)
  add a test for scatter colorbar extend (pydata#7616)
  ...
@TomNicholas
Copy link
Member

Thanks @harshitha1201 ! A great usability improvement.

@harshitha1201
Copy link
Contributor Author

Thanks @harshitha1201 ! A great usability improvement.

Thanks!

dcherian added a commit to kmsquire/xarray that referenced this pull request Mar 29, 2023
* upstream/main: (716 commits)
  Faq pull request (According to pull request pydata#7604 & issue pydata#1285 (pydata#7638)
  add timeouts for tests (pydata#7657)
  Pull Request Labeler - Undo workaround sync-labels bug (pydata#7667)
  [pre-commit.ci] pre-commit autoupdate (pydata#7651)
  Allow all integer dtypes in `polyval` (pydata#7619)
  [skip-ci] dev whats-new (pydata#7660)
  Redo whats-new for 2023.03.0 (pydata#7659)
  Set copy=False when calling pd.Series (pydata#7642)
  Pin pandas < 2 (pydata#7650)
  Whats-new for release 2023.03.0 (pydata#7643)
  Bump pypa/gh-action-pypi-publish from 1.7.1 to 1.8.1 (pydata#7648)
  Use more descriptive link texts (pydata#7625)
  Fix missing 'dim' argument in _get_nan_block_lengths (pydata#7598)
  Fix `pcolormesh` with str coords (pydata#7612)
  [skip-ci] Fix groupby binary ops benchmarks (pydata#7603)
  Remove incomplete sentence in IO docs (pydata#7631)
  Allow indexing unindexed dimensions using dask arrays (pydata#5873)
  Bump pypa/gh-action-pypi-publish from 1.6.4 to 1.7.1 (pydata#7618)
  [pre-commit.ci] pre-commit autoupdate (pydata#7620)
  add a test for scatter colorbar extend (pydata#7616)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Outreachy-2023 Participation in Outreachy.org 2023 plan to merge Final call for comments topic-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants