Skip to content

Missing documentation targets after refactor that moved classes out of core module #10195

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

Closed
malmans2 opened this issue Apr 1, 2025 · 10 comments · Fixed by #10207
Closed

Missing documentation targets after refactor that moved classes out of core module #10195

malmans2 opened this issue Apr 1, 2025 · 10 comments · Fixed by #10207

Comments

@malmans2
Copy link
Contributor

malmans2 commented Apr 1, 2025

Is your feature request related to a problem?

I've been using DataArrayWeighted and DatasetWeighted for typing purposes. A recent refactor moved these objects from xarray.core.weighted to xarray.computation.weighted, which broke my downstream code. Technically, I guess this is not a breaking change, but those classes are exposed in the documentation (e.g., DataArrayWeighted)

Would it be possible to import these objects at the top level in __init__.py? If not, what would you recommend for downstream packages? Should they just avoid relying on these classes?

Describe the solution you'd like

from xarray import DataArrayWeighted, DatasetWeighted

Describe alternatives you've considered

No response

Additional context

No response

@EddyCMWF
Copy link

EddyCMWF commented Apr 1, 2025

I also noticed this withDataArrayRolling , this was previously here: xarray.core.rolling it is now here: xarray.computation.rolling

@malmans2
Copy link
Contributor Author

malmans2 commented Apr 1, 2025

Another thing I just noticed, many docstrings are pointing to the old modules. For example:

core.weighted.DataArrayWeighted

@max-sixty
Copy link
Collaborator

we were thinking of putting these in an xarray.typing module, what are your thoughts?

there's a benefit to having a cleaner top-level namespace, so unless there's no way around that, I'd vote to exclude them

we should clean up the docstrings; I'm surprised the docs don't fail. would def take a PR, but if not I'll try and fix soon

@malmans2
Copy link
Contributor Author

malmans2 commented Apr 1, 2025

Assuming users are not supposed to directly instantiate those classes, yes I think xarray.typing would work well.

If there's no rush, I'm happy to fix the docstrings in the next few days.

@max-sixty
Copy link
Collaborator

thanks a lot @malmans2 !

@keewis
Copy link
Collaborator

keewis commented Apr 1, 2025

I'm surprised the docs don't fail

the docs don't fail because the outdated import paths result in broken links, and we'd need to enable nitpicky mode to get errors for that. As far as I can tell, we inherit too many docstrings from pandas / numpy that we don't have any control over, so unless we refactor I'm not sure if we can make nitpicky mode to pass so that that check is actually useful.

@max-sixty
Copy link
Collaborator

the docs don't fail because the outdated import paths result in broken links, and we'd need to enable nitpicky mode to get errors for that.

ah thanks. and we can't do this for our docstrings only?

@keewis
Copy link
Collaborator

keewis commented Apr 1, 2025

At least I couldn't figure out how to do that back when I looked into this (which was a couple of years ago, so something might have changed).

However, the point is that we do want to document e.g. CFTimeIndex's methods, which is where most of the problematic type spec strings are (we do have some of our own, but not that many).

@max-sixty
Copy link
Collaborator

that makes sense, thanks @keewis

@malmans2
Copy link
Contributor Author

malmans2 commented Apr 7, 2025

Hi all,

I realised there's a similar issue covering this topic: #10179
I'm going to edit the title of this one, since we've been focusing on docstrings.

I'll open a PR to close this issue later. In the meantime, I've compiled the docs with these variables:

nitpicky = True

nitpick_ignore_regex = [
    # Ignore all roles that are not py:class
    (r"py:(?!class).*", r".*"),
    # Ignore all py:class references that do NOT contain "core."
    (r"py:class", r"^(?!.*core\.).*"),
]

Which I think does the job and returns the list of classes I should fix in the PR:

/Users/mattia/MyGit/xarray/doc/generated/xarray.DataArray.rst:174:<autosummary>:1: WARNING: py:class reference target not found: xarray.core.accessor_dt.CombinedDatetimelikeAccessor
/Users/mattia/MyGit/xarray/xarray/core/dataarray.py:docstring of xarray.core.dataarray.DataArray.coarsen:11: WARNING: py:class reference target not found: core.rolling.DataArrayCoarsen
/Users/mattia/MyGit/xarray/xarray/core/dataarray.py:docstring of xarray.core.dataarray.DataArray.coarsen:106: WARNING: py:class reference target not found: core.rolling.DataArrayCoarsen
/Users/mattia/MyGit/xarray/xarray/core/dataarray.py:docstring of xarray.core.dataarray.DataArray.cumulative:8: WARNING: py:class reference target not found: core.rolling.DataArrayRolling
/Users/mattia/MyGit/xarray/xarray/core/dataarray.py:docstring of xarray.core.dataarray.DataArray.rolling:13: WARNING: py:class reference target not found: core.rolling.DataArrayRolling
/Users/mattia/MyGit/xarray/xarray/core/dataarray.py:docstring of xarray.core.dataarray.DataArray.weighted:12: WARNING: py:class reference target not found: core.weighted.DataArrayWeighted
/Users/mattia/MyGit/xarray/xarray/core/dataset.py:docstring of xarray.core.dataset.Dataset.coarsen:11: WARNING: py:class reference target not found: core.rolling.DatasetCoarsen
/Users/mattia/MyGit/xarray/xarray/core/dataset.py:docstring of xarray.core.dataset.Dataset.coarsen:15: WARNING: py:class reference target not found: core.rolling.DatasetCoarsen
/Users/mattia/MyGit/xarray/xarray/core/dataset.py:docstring of xarray.core.dataset.Dataset.cumulative:8: WARNING: py:class reference target not found: core.rolling.DatasetRolling
/Users/mattia/MyGit/xarray/xarray/core/dataset.py:docstring of xarray.core.dataset.Dataset.rolling:13: WARNING: py:class reference target not found: core.rolling.DatasetRolling
/Users/mattia/MyGit/xarray/xarray/core/dataset.py:docstring of xarray.core.dataset.Dataset.weighted:12: WARNING: py:class reference target not found: core.weighted.DatasetWeighted
/Users/mattia/MyGit/xarray/doc/internals/how-to-create-custom-index.rst:56: WARNING: py:class reference target not found: xarray.core.indexes.PandasIndex
/Users/mattia/MyGit/xarray/doc/internals/how-to-create-custom-index.rst:56: WARNING: py:class reference target not found: xarray.core.indexes.PandasMultiIndex
/Users/mattia/MyGit/xarray/doc/internals/internal-design.rst:91: WARNING: py:class reference target not found: xarray.core.Variable
/Users/mattia/MyGit/xarray/doc/internals/internal-design.rst:110: WARNING: py:class reference target not found: xarray.core.Variable
/Users/mattia/MyGit/xarray/doc/internals/interoperability.rst:13: WARNING: py:class reference target not found: xarray.core.parallelcompat.ChunkManagerEntrypoint
/Users/mattia/MyGit/xarray/doc/user-guide/combining.rst:25: WARNING: py:class reference target not found: xarray.Dataset`s / :py:class:`~xarray.DataArray`s along an existing or new dimension
into a larger object, you can use :py:func:`~xarray.concat
/Users/mattia/MyGit/xarray/doc/whats-new.rst:483: WARNING: py:class reference target not found: xr.core.parallelcompat.ChunkManagerEntrypoint
/Users/mattia/MyGit/xarray/doc/whats-new.rst:1870: WARNING: py:class reference target not found: xr.core.parallelcompat.ChunkManagerEntrypoint

@malmans2 malmans2 changed the title Import DataArrayWeighted and DatasetWeighted at top level Missing documentation targets after refactor that moved classes out of core module Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants