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

Extending the glossary #7732

Merged
merged 23 commits into from
Aug 18, 2023
Merged

Extending the glossary #7732

merged 23 commits into from
Aug 18, 2023

Conversation

harshitha1201
Copy link
Contributor

Added align, broadcast, merge, concatenate and combine to terminology.
-> Closes #3355

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.

That is an important addition, the different words and their meaning are always confusing for beginners.

Maybe we could add some very simple examples to demonstrate the effects (1D or 2D arrays).

In general I think you could find better words for "combine", basically every definition uses it.

doc/user-guide/terminology.rst Show resolved Hide resolved
doc/user-guide/terminology.rst Show resolved Hide resolved
doc/user-guide/terminology.rst Show resolved Hide resolved
doc/user-guide/terminology.rst Show resolved Hide resolved
@headtr1ck
Copy link
Collaborator

headtr1ck commented Apr 7, 2023

Actually on top of examples maybe some simple text depictions could be useful?
Something like this for aligning:
Values: [ A A A ] + [ B B ] -> [ A B A B A ]
Index:. [ 1 3 5 ] + [ 2 4 ] -> [ 1 2 3 4 5 ]

But just throwing ideas around

Edit: that's actually not what aligning does, haha. But you get the idea.

@harshitha1201
Copy link
Contributor Author

@headtr1ck I have done the changes required, 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. I still have several remarks.

Also, the indentation of the compiled docpage looks wrong (see https://xray--7732.org.readthedocs.build/en/7732/user-guide/terminology.html)
After a code block the following text is no longer indented. I have no idea how this exactly works though...

doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
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 looks great, but none of the new definitions are visible in the rendered docs page. I think this is just an indentation problem.

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.

Great PR! Some things left to clarify.

@headtr1ck
Copy link
Collaborator

Any reason why you removed the examples?
I didn't follow the review process if this was requested by someone.

@harshitha1201
Copy link
Contributor Author

Any reason why you removed the examples? I didn't follow the review process if this was requested by someone.

I was unable to build the examples while doing make html, but now I'm able to. I'll add them again.

you're ready to see the final result, you tell xarray, "Okay, go ahead and do those calculations now!"
That's when xarray starts working through the steps you planned and gives you the answer you wanted.This
lazy approach helps save time and memory because xarray only does the work when you actually need the
results.
Copy link
Member

Choose a reason for hiding this comment

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

Lazy evaluation is provided alternately by dask or by hidden xarray internals, depending on whether dask is installed. I'm wondering whether it's worth mentioning that here or not. @headtr1ck what do you think? I've added it to #7991 so we could link to that page?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can leave this clarification for later.

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.

Great work on this!

General comment 1: The definitions are clearest when they start with a single short sentence succinctly describing the term. Then afterwards you can write a paragraph explaining the context or why that's useful. Look at the entries for DataArray etc. to see what I mean.

General comment 2: We could be adding links to other relevant sections of the documentation. For example the entry on indexing should link to the docs page on indexing.

doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
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 ! This is a great addition!

@TomNicholas TomNicholas added the plan to merge Final call for comments label Aug 18, 2023
@dcherian
Copy link
Contributor

Thanks @harshitha1201

@dcherian dcherian merged commit 4c07a01 into pydata:main Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define aligning, broadcasting, merging, concatenating, combining in new glossary
4 participants