issue_comments

Hi,

I assume you have given this a lot of thought, but imho the minimum dependency versions should be decided according to features needed, not timing.

It's not based on timing. The policy is there so that, when a developer finds that they have to do extra labour to support an old version of a dependency, they can instead drop the support for the old version without needing to seek approval from the maintainers.

That's not how I interpret the link given by @dcherian, which states "rolling" minimum versions based on age.

Hi @dcherian

Here is our support policy for versions: https://docs.xarray.dev/en/stable/getting-started-guide/installing.html#minimum-dependency-versions though I think we dropped py38 too early.

I assume you have given this a lot of thought, but imho the minimum dependency versions should be decided according to features needed, not timing.

For your current issue, I'm surprised this patch didn't fix it: conda-forge/conda-forge-repodata-patches-feedstock#429

Thanks for the pointer. I am not sure why, maybe I was updating too eagerly before the feedstock was fixed, but mamba update --all on py38 pulled pandas 2.0 without updating xarray.

python3.8 -m pip install xarray will result in incompatible versions.

cc @hmaarrfk @ocefpaf

Hi, Just to let you know that this change breaks python 3.8 setups with automatic updates becuase the pandas version is not restricted, so it will be happily updated to version 2 or higher. Which in turn is not compatible with xarray < 2023.2, which cannot be installed on python 3.8 because of this change. Don't know why the min python version was changed, this PR doesn't say why it was necessary. Cheers.

Hi @mathause

It does work if the array keeps the size:

python data.resample(index="M").apply(lambda x: x.values)

Thanks, but I am not sure I find that intuitive, why should the resampled array have the same size as the original? It seems to make only sense for DataArray.apply(), but not for a resampled one. As I indicated in my other reply, returning a scalar or equivalent should be fine, shouldn't it? At the very least the documentation is lacking, it refers to the pandas method, but clearly the behaviour is different.

As a workaround you could allow your function to consume a dummy axis. Or you could pass dim as ...

python data.resample(index="M").reduce(lambda x, axis: 1) # workaround 1 data.resample(index="M").reduce(lambda x: 1, dim=...) # workaround 2

(reduce only passes axis if dim is not None but groupby passes the group_dim per default.

That feels a bit like curing the symptoms instead of the root cause, why not set dim='...' if it is not given?

@mathause Thanks, that works for np.median, but .reduce() passes an axis= keyword argument, which needs to be taken care of by any custom function. I used np.median here just as an example, as it showed the problem.

@dcherian I am not sure if raising an error would be very user friendly. In my opinion, which may be biased by my personal use cases, I would expect .apply() (or .map() for that matter) of a resample object to take any function that returns a scalar: the value at the resampled point. It should be then up to xarray to iterate over any other non-resampled dimensions. I am not sure why it requires to return a DataArray in the first place. As mentioned, it works with pandas series and dataframes, e.g.:

python data.to_pandas().resample("M").apply(np.median)

@Illviljan Thanks for this comparison, I certainly prefer the second one, it looks better aligned to me and we get a little more information on one page. But then that's only me. ;)

Edit: Not sure what you mean with the attributes, they all start in the 5th column, after 4 spaces like the officially recommended python indentation.

The tests look good!

Any thoughts from anyone before we merge?

Looks good, but in my opinion this wastes some screen space on the left side. One could probably start a new line for the dimensions in a Dataset: python Dimensions: (long_coord_name_0_x: 20, long_coord_name_10_x: 20, ... or with some indenting: python Dimensions: (long_coord_name_0_x: 20, long_coord_name_10_x: 20, ... Maybe the same for the DataArray: python <xarray.DataArray 'LongDataArrayName' (dim_0: 2, dim_1: 2, dim_2: 2, dim_3: 2, dim_4: 2, dim_5: 2, dim_6: 2, dim_7: 2, dim_8: 2, dim_9: 2, dim_10: 2, dim_11: 2)> Just an idea.

@keewis Thanks for the pointers, I'd say that nothing public facing should change in 0.18 now. OT (edit): By the way, these incompatibilities happen when one side decides to change the API without considering that some users may actually use that interface (and looking at the pandas' "deprecation" list, I fear that this will only get worse). Nice from the xarray people to have a section about keeping backwards compatibility as much as possible in their contribution guidelines.

As for the tests, I found the tests that @max-sixty put in and extended them (see second and third commits in this PR). However, now there is one dataset setup and then 4(!) asserts, which seems to be too much to follow nicely. Imagine all of them break, you fix the first, only to find out that the second breaks as well. so you fix that, only to find out that the third breaks too, and so on.

@Illviljan It is a good idea, however, I'd prefer if those changes were introduced as an option first, before changing the default behaviour.

Hi @max-sixty, Sure, but it will take a bit. Could you point me to right places for the docs? Just the filenames would do.

I would be in favour of waiting a little with this change to get a few more opinions. This change will ignore display_max_rows for everything except dataset.__repr__, which may not be what some people might expect or hope. Any ideas how to get more people to weigh in?

Edit: I would also like to separate the tests, to make it easier to follow if something breaks, but the setup for the test dataset would be the same. Any preferences or best practices for the code layout in such a case without duplicating too much of the code? However, that can probably wait for another PR.

Hi, @max-sixty I could give it a try, but my time is quite limited. Would you be fine with a diff? That would save me a bit from setting up a fork and new repo.

Anyway, here is a quick diff, I tried to keep it small and basically moved the max_rows setting to dataset_repr, only coords_repr takes a new keyword argument, so that should be backwards compatible. The tests would need to be updated. Maybe it is a good idea to not test _mapping_repr, but instead test coords_repr, data_vars_repr, attrs_repr, and dataset_repr separately, to check that they do what they are supposed to do regardless of their implementation?

Edit: Never mind, I am preparing a PR with updated tests.

```diff diff --git a/xarray/core/formatting.py b/xarray/core/formatting.py index 07864e81..ab30facf 100644
--- a/xarray/core/formatting.py
+++ b/xarray/core/formatting.py @@ -377,14 +377,12 @@ def _mapping_repr(
):
if col_width is None: col_width = _calculate_col_width(mapping) - if max_rows is None:
- max_rows = OPTIONS["display_max_rows"]
summary = [f"{title}:"] if mapping:
len_mapping = len(mapping) if not _get_boolean_with_default(expand_option_name, default=True): summary = [f"{summary[0]} ({len_mapping})"] - elif len_mapping > max_rows:
+ elif max_rows is not None and len_mapping > max_rows:
summary = [f"{summary[0]} ({max_rows}/{len_mapping})"] first_rows = max_rows // 2 + max_rows % 2 items = list(mapping.items())
@@ -416,7 +414,7 @@ attrs_repr = functools.partial(
)

-def coords_repr(coords, col_width=None):
+def coords_repr(coords, col_width=None, max_rows=None):
if col_width is None: col_width = _calculate_col_width(_get_col_items(coords)) return _mapping_repr( @@ -425,6 +423,7 @@ def coords_repr(coords, col_width=None): summarizer=summarize_coord, expand_option_name="display_expand_coords", col_width=col_width, + max_rows=max_rows, ) @@ -542,21 +541,22 @@ def dataset_repr(ds): summary = ["<xarray.{}>".format(type(ds).name)]

 col_width = _calculate_col_width(_get_col_items(ds.variables))

• max_rows = OPTIONS["display_max_rows"]

  dims_start = pretty_print("Dimensions:", col_width) summary.append("{}({})".format(dims_start, dim_summary(ds)))

  if ds.coords: - summary.append(coords_repr(ds.coords, col_width=col_width)) + summary.append(coords_repr(ds.coords, col_width=col_width, max_rows=max_rows))

  unindexed_dims_str = unindexed_dims_repr(ds.dims, ds.coords) if unindexed_dims_str: summary.append(unindexed_dims_str)

• summary.append(data_vars_repr(ds.data_vars, col_width=col_width))

• summary.append(data_vars_repr(ds.data_vars, col_width=col_width, max_rows=max_rows))

  if ds.attrs: - summary.append(attrs_repr(ds.attrs)) + summary.append(attrs_repr(ds.attrs, max_rows=max_rows))

  return "\n".join(summary)

```

@benbovy That sounds good to me. If I may add, I would leave __repr__ and __str__ to return the same things, since people seem to use them interchangeably, e.g. in tutorials, and probably in their own code and notebooks.

@max-sixty I apologize if I hurt someone, but it is hard to find a solution if we can't agree on the problem. Try the same examples with 50 or 100 instead of 2000 variables to understand what I mean. And to be honest, I found your comments a bit dismissive and not exactly welcoming too, which is probably also not your intention.

From what I see in the examples by @Illviljan , setting display_max_rows affects everything equally, coords, data_vars, and attrs. So there would be no need to treat them separately. Or I misunderstood your comment.

Anyway, I think I made my point, I leave it up to you to decide what you are comfortable with.

Hi @Illviljan, As I mentioned earlier, your "solution" is not backwards compatible, and it would be counterproductive to update the doctest. Which is also not relevant here and a different issue.

I am not sure what you are trying to show, your datasets look very different from what I am working with, and they miss the point. Then again they also prove my point, pandas and numpy shorten in a canonical way (except the finite number of columns, which may make sense, but I don't like that either and would rather have it wrap but show all columns). xarray doesn't because usually the variables are not simply numbered as in your example.

I am talking about medium sized datasets of a few 10 to maybe a few 100 non-canonical data variables. Have a look at http://cfconventions.org/ to get an impression of real-world variable names, or the example linked above in comment https://github.com/pydata/xarray/issues/5545#issuecomment-870109486. There it would be nice to have an overview over all of them.

If too many variables are a problem, imo it would have been better to say: "We keep it as it is, however, if it is a problem for your large dataset, here is an option to reduce the amount of output: ..." And put that into the docs or the wiki or FAQ or something similar. Note that the initial point in the linked issue is about the time it takes to print all variables, not the amount that gets shown. And usually the number of coordinates and attributes is smaller than the number of data variables. It also depends on what you call "screen", my terminal has currently 48 lines (about 56 in fullscreen, depending on fontsize), and a scrollback buffer of 5000 lines, I am also used to scrolling through long jupyter notebooks. Scrolling through your examples might be tedious (not for me actually), but I will never be able to find typos hidden in the three dots.

@max-sixty No worries, I understand that this is a minor cosmetic issue, actually I intended it as a feature request, not a bug. But that must have gone missing along the way. I guess I could live with 50, any other opinions? I am sure someone else will complain about that too. ;)

Hi @max-sixty

We need to cut some of the output, given a dataset has arbitrary size — same as numpy arrays / pandas dataframes.

I thought about that too, but I believe these cases are slightly different. In numpy arrays you can almost guess how the full array looks like, you know the shape and get an impression of the magnitude of the entries (of course there can be exceptions which are not shown in the output). Similar for pandas series or dataframes, the skipped index values are quite easy to guess. The names of data variables in a dataset are almost impossible to guess, as are their dimensions and data types. The ellipsis is usually used to indicate some kind of continuation, which is not really the case with the data variables.

If people feel strongly about a default > 12, that seems reasonable. Do people?

I can't speak for other people, but I do, sorry about that. @shoyer 's suggestion sounds good to me, from the top of my head 30-100 variables in a dataset seems to be around what I have come across as a typical case. Which does not mean that it is the typical case.

I switched off html rendering altogether because that really slows down the browser, haven't had any problems with the text output. The text output is (was) also much more concise and does not require additional clicks to open the dataset and see which variables are in there.

The problem with your suggestion is that this approach is not backwards compatible, which is not nice towards long-term users. A larger default would be a bit like meeting half-way. I also respectfully disagree about the purpose of __repr__(), see for example https://docs.python.org/3/reference/datamodel.html#object.repr . Cutting the output arbitrarily does not allow one to "recreate the object".

Why not increase that number to a more sensible value (as I suggested), or make it optional if people have problems? If people are concerned and have problems, then this would be an option to fix that, not the other way around. This enforces such a low limit onto all others.

Hi, Thanks for the replies, I was indeed caught by surprise and given that the version number is 0.11.x, I had the impression that 0.12.x would be the next major minor release (and coming soon).

@shoyer In that case I would take it back and vote for a change as soon as possible to stabilize the API. Although xarray is still considered beta, I guess some people already use it productively.

@jhamman Thanks for the offer, I think the changes were simple enough. I merely wanted to point out that some more people use(d) that feature.

Hi, Sorry for the late comment about a closed bug. But I find changing the API a bit irritating to say the least, and this is a serious change. Although apparently not many people use it, some actually may (myself included). And so far there has been only one bug report, so what problem are you trying to fix? I can fix my own code but there may be others out there that cannot keep pace with the development and including their packages may then break software. For my taste the deprecation warning is a bit short if you are going to remove such a feature already in the next version. A few more cycles would be appreciated.

At the very least put a big warning sign to the documentation that xarray is still beta and the API is still subject to change.

Hi, just to let you know that .std() does not accept the ddof keyword anymore (it worked in 0.10.8) Should I open a new bugreport?

Edit: It fails with:

~/Work/miniconda3/envs/stats/lib/python3.6/site-packages/xarray/core/duck_array_ops.py in f(values, axis, skipna, **kwargs)
    234 
    235         try:
--> 236             return func(values, axis=axis, **kwargs)
    237         except AttributeError:
    238             if isinstance(values, dask_array_type):

TypeError: nanstd() got an unexpected keyword argument 'ddof'