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

Big docs reorganise and expand. #109

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Big docs reorganise and expand. #109

wants to merge 22 commits into from

Conversation

pp-mo
Copy link
Owner

@pp-mo pp-mo commented Jan 16, 2025

Closes #80
Refactored version of #105

Adds new sections on core data and general operations.
Integrates the how-to sections also

Todos:

  • prune for repetition.
  • review + clarify 4-way Diataxis-like top-level division =
    • Getting Started (intro + tutorial)
    • User Guide
    • API
    • Detail Notes
  • move existing details sections to "detail notes" sections (possibly with short hinting links)
  • add a how-to on chunking control
  • review for changes to equality provision (no comprehensive == on variable/ncdata)
  • re-proof-read all

@pp-mo pp-mo mentioned this pull request Jan 16, 2025
4 tasks
@pp-mo pp-mo changed the title Big rework and expand docs. Big docs reorganise and expand. Jan 16, 2025
@pp-mo pp-mo requested a review from trexfeathers January 29, 2025 10:13
@pp-mo
Copy link
Owner Author

pp-mo commented Jan 29, 2025

@trexfeathers welcome and thanks for looking !
if reviewing, scope here is really : do the docs structure and coverage generally look appropriate to you now (as a first proper attempt). i.e. notably

  • are there things badly misplaced
  • or plain missing
  • is it clear enough from the entry point level what is available + where to find things

Great also to have anyone else comment on this,
I think it's the only thing really needed now prior to v0.2 release (except release whatsnew drafting)

@pp-mo
Copy link
Owner Author

pp-mo commented Feb 6, 2025

OK I have now reviewed all the API docs builds, fixed (hopefully) for correctness and updated, generally smoothed over and introduced some more cross-links into newer docs sections where it seemed obvious.

So I reckon this is up to scratch now, pending any suggestions about structural improvements.

Copy link
Collaborator

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

OK I've read most of it. It is impressively comprehensive 💐

To-do for @trexfeathers:

  • Review the details section
  • Consider the overall structure/approachability

docs/userdocs/user_guide/data_objects.rst Outdated Show resolved Hide resolved
docs/userdocs/user_guide/data_objects.rst Outdated Show resolved Hide resolved
docs/userdocs/user_guide/data_objects.rst Outdated Show resolved Hide resolved
docs/userdocs/user_guide/data_objects.rst Outdated Show resolved Hide resolved
docs/userdocs/user_guide/common_operations.rst Outdated Show resolved Hide resolved
Comment on lines +517 to +519
>>> if "_fillvalue" in var.attributes:
>>> var.attributes.rename("_fillvalue", "_FillValue")
...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing indenting

docs/userdocs/user_guide/howtos.rst Outdated Show resolved Hide resolved

Accepts paths, pathstrings, open :class:`netCDF4.Dataset`\\s or :class:`NcData` objects.
Accepts paths, pathstrings, open :class:`netCDF4.Dataset`\s or :class:`NcData`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sphinx domains not rendering

Copy link
Owner Author

Choose a reason for hiding this comment

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

Coincidentally, just fixed this docstring in a parallel PR : #112

(should now come good when this is rebased or merged from main)

lib/ncdata/threadlock_sharing.py Outdated Show resolved Hide resolved
docs/details/known_issues.rst Outdated Show resolved Hide resolved
@pp-mo pp-mo mentioned this pull request Feb 6, 2025
Copy link
Collaborator

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Here is the rest of my review.

General thoughts

I think this is some of the most complete documentation we have, because it caters to all the different types of readers.

It is laid out in an approachable way that is also low maintenance, so I'm happy to merge with the current structure 👍👍👍

However

Given the excellent breadth of 'all-angles' content, it feels like a missed opportunity to not go all-in on Diataxis. From offline conversations this sounds deliberate, but I'm hoping we can come back in future to 'finish the job'?

docs/details/interface_support.rst Outdated Show resolved Hide resolved
docs/details/interface_support.rst Outdated Show resolved Hide resolved
docs/details/interface_support.rst Outdated Show resolved Hide resolved
docs/details/threadlock_sharing.rst Outdated Show resolved Hide resolved
In practice, Iris, Xarray and Ncdata are all capable of scanning netCDF files and interpreting their metadata, while
not reading all the core variable data contained in them.

This generates objects containing `Dask arrays <https://docs.dask.org/en/stable/array.html>`_ with deferred access
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have Intersphinx for Dask, so I recommend using it.

To achieve a link to this specific page, you can use this syntax (not sure about correct way to pluralise):

Suggested change
This generates objects containing `Dask arrays <https://docs.dask.org/en/stable/array.html>`_ with deferred access
This generates objects containing Dask :external+dask:doc:`array` s with deferred access

More:

docs/details/threadlock_sharing.rst Show resolved Hide resolved
docs/change_log.rst Show resolved Hide resolved
docs/userdocs/user_guide/common_operations.rst Outdated Show resolved Hide resolved
docs/userdocs/user_guide/howtos.rst Outdated Show resolved Hide resolved
Comment on lines 170 to 173
>>> variable.set_attr("x", 3.)
>>> variable.get_attr("x")
3.0
>>> variable.set_attr("x", "string-value")
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's not the same as __setattr__ , because that would be like a variable.x = 'string-value'

But this is a variable.attributes["x"].value = 'string-value'

Confusing, eh ?!

OK then __setitem__. Ignoring my technical inaccuracy, I'm still interested about whether this design philosophy is a potential direction to explore in future, or whether it's just impossible. The current need for documentation shows that we're not adhering to the least-surprise principle.

@pp-mo
Copy link
Owner Author

pp-mo commented Feb 11, 2025

Progress : New commit 3433c29 covers, I think, all the "original" review comments
- and just a couple of the newer set
I have to stop now, and it makes a good point to pause.

Meanwhile I start to think about the "newer set" of suggestions

@pp-mo
Copy link
Owner Author

pp-mo commented Feb 12, 2025

@trexfeathers thanks for your careful attention !
I think I've now addressed all-to-date

Probable exceptions, issues you've raised which I think I want to "pin" for now :

  • recognise diataxis explicitly
  • consider rename, replace or remove the "set_attrname" and "get_attrname" methods
  • update changenotes including release version Complete changenotes for v0.2 #115

Please check it out + see what you think.

On inspection, I think I probably don't need to merge from main before completing this (read on...)
So easier for you to check it all out as-is without further re-hash, and I'll merge from main later.

Differences currently waiting on main are only those from #112, to do with unpinning numpy to v2. Associated changes affect array printout and its tests. That does not create major problems here, as changes here are all docs with no functional effect.
It may affect some code examples, but as they don't run as doctests, we will only find that later by looking.
Not exactly a good thing, but not a merge blocker !

@pp-mo
Copy link
Owner Author

pp-mo commented Feb 12, 2025

Hang on ....

I just realised the initial "Introduction" tutorial has chunks of code that ought to be in rst "code-block"s.
Left-overs from my initial ignorance.
I will attend to that ...

@pp-mo
Copy link
Owner Author

pp-mo commented Feb 12, 2025

Hang on ....

I just realised the initial "Introduction" tutorial has chunks of code that ought to be in rst "code-block"s. Left-overs from my initial ignorance. I will attend to that ...

OK done that. Not sure what else I'm still missing at the last minute though 😥

Copy link
Collaborator

@trexfeathers trexfeathers 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 great! We're down to 4 outstanding options, plus a final 5th conversation that needs no action.

Comment on lines +407 to +410
>>> for name in wanted:
... data.variables.add(data.variables[name])
...
>>> to_nc4('output.nc')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
>>> for name in wanted:
... data.variables.add(data.variables[name])
...
>>> to_nc4('output.nc')
>>> for name in wanted:
... data.variables.add(data2.variables[name])
...
>>> to_nc4(data, 'output.nc')

Very briefly :

* types (1) and (2) are equivalent to Python strings and may include unicode
* type (2) are equivalent to character (byte) arrays, and normally represent only
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* type (2) are equivalent to character (byte) arrays, and normally represent only
* type (3) are equivalent to character (byte) arrays, and normally represent only

Comment on lines 170 to 173
>>> variable.set_attr("x", 3.)
>>> variable.get_attr("x")
3.0
>>> variable.set_attr("x", "string-value")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think?

I was gonna make a branch to illustrate, but I think that would need too much time.

The current handling of self.attributes is moved into self._attributes. You no longer have to warn people against working with it directly - it's implicit/self-explaining, especially since it's undocumented. But it's still easily accessible for ncdata developers, and is the object that is in charge.

A new self.attributes is created as the 'public face'. This has a __setitem__ and __getitem__, which perform the work currently done by set_attrval get_attrval, writing/reading from self._attributes.

IMO this would be the best of both worlds.


I consider my original comment to be actioned. Please feel free to Resolve conversation once you have read this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation extensions
2 participants