issue_comments

Is there anything I can do to help out on this? It sounds like the blocker is mypy?

to properly test this, I guess we'd need to merge #7724 first?

Otherwise the env in the upstream test will never solve right?

In that case it could be reasonable to mimic the pattern in test_groupby and mark the failing tests with a @pytest.mark.skipif(not has_pandas_version_two, reason="Tests a scenario that only raises when pandas <= 2")

My understanding is that indexes have come a long way since this PR was last touched. Maybe now is the right time to rewrite this in a way that is more performant?

I should say that there might very well be other uses of load that I am not aware of. The inplace vs not is just the only difference that I noticed in the context of dask-backed xarray objects.

From this discussion I think for dask-backed objects we should recommend using compute. Maybe that is enough of a decision.

I can imagine a scenario where load is de-emphasized in the narrative docs, and compute is used instead. It wouldn't have to be a formal deprecation, but it could be that compute is best practice.

Thanks for engaging with this! I think removing load probably makes the most sense. Making load an alias for compute would just continue the confusion since there wouldn't be a clear right way to do things.

I somehow assumed compute would only work with dask while load would also load our lazy array implementation into memory. Not sure how I got that impression, but maybe that's another argument to remove / align load?

I'm not quite sure I follow. Are there lazy array implementations other than dask in xarray?

I think this might break open_mfdataset where some dimensions only exist in some files but we want to chunk along them

ah. So maybe there isn't anything interesting to do here? It's not very important.

I'm sorry I missed pings on this. I am happy to join the dev meeting if that'd be helpful or I can just start paying better attention :roll_eyes:

I'm fine with this being closed. TBH I had totally forgotten about it :grimacing:

I didn't manage to reproduce the lag, but this change seems totally reasonable to me!

I'm glad you like it! @benbovy really did all the work on writing it. I think there was some talk of extracting it to a separate library, but I'm not sure if it was obvious how to generalize it. It depends whether you would want it to be a standalone library (like pprint) or whether you'd want the libraries to integrate with it (probably using entrypoints).

I would start by thinking about one or two other classes that you'd like to see represented this way and try to see how well what currently exists generalizes.

I'm looking at the relevant test function:

python def test_repr_of_dataarray(dataarray): formatted = fh.array_repr(dataarray) assert "dim_0" in formatted # has an expanded data section assert formatted.count("class='xr-array-in' type='checkbox' checked>") == 1 # coords and attrs don't have an items so they'll be be disabled and collapsed assert ( formatted.count("class='xr-section-summary-in' type='checkbox' disabled >") == 2 )

does this only check on the number of collapsed sections?

Yes, that test counts expanded data sections and collapsed summary sections

How could I check specifically if the data section is collapsed based on the OPTION setting?

Use set_option as a context manager and do a test much like the one above.

I think dask is going to release today.

Working on a fix now. I think https://github.com/dask/dask/pull/6299 should do it.

Yes that seems like the explanation. Thanks @benbovy! I would expect dask to have the same behavior then since they pass in the style tag in the same way.

Hmm. Ok. I'm still not sure why the html would get messed up under that scenario though.

Does it seem possible that you had originally run it in jlab and then opened it in notebook or vice versa? I've seen html output get kind of messed up under those conditions, but I've never really stopped to debug it thoroughly.

It seems like the auto-update thing works for css, but not for icons.

Huh and if you re-run cell 69 does it render correctly? I'm on chrome 80, but if you were seeing this in firefox as well it seems like it's more likely to be a jupyterlab thing.

I can't reproduce:

Tried on chrome and firefox on linux. Can you inspect the element in your browser console and see if there is a <style></style> section for the repr? It seems like that might not be getting in properly

Yeah I can take a look now.

I am fairly confident that the failures are not related to the changes in this PR

Is this ready to merge?

Yeah I think so.

Right I don't think jlab works in IE. But this won't work in regular notebook in IE either.

I don't really like the hover-background anyways if those rows aren't clickable and other things are.

I think this can be merged as is.

Besides variable names, we probably need to somehow handle overflow for variables that have a long list of dimension labels.

These are handled in the same way as long variable names, they truncate with an ellipsis and then expand on hover.

I opened a PR and included a small test. It can probably go in while we work on a more complete testing infrastructure.

I looks like we'll need to fix a couple of issues (the channel coordinate looks weird), but maybe it is not related to the front-end.

I am opening a PR. I didn't escape dtypes and this dtype is <U5 which html does terribly with

@benbovy in case you have ideas about which colors to use.

Note that this won't work in IE https://caniuse.com/#feat=css-variables

It isn't too hard to get it working with dark theme, but requires slight tweaks to the colors to align with those provided as vars by jlab.

Ok I think it's fixed now

I'm on 1.1.4 in chrome. Let me check firefox

Yeah to make it work with the dark theme we'll need to use the jlab css variables I imagine. I think it can be merged with dark mode not supported.

Fixed the jlab alignment issue. The issue was that jlab has different default css.

I've noticed an issue with the alignment of attributes in jupyterlab (1.1.4) (Firefox 69.0.3):

I'm taking a look at the jlab css.

@benbovy just to make double sure, you are happy with this new feature being off by default right?

So maybe one more rebase will do it?

Well, I rebased. Hopefully it'll pass now. I consider this done unless someone wants more docs or more tests.

I am 90% sure this failure is unrelated. Maybe I should rebase now that #3436 has landed?

Oh. The dask version in ci doesn't have _repr_html_. I'll add a version checker to the test.

Otherwise I think all that is left is adding docs. Also I want to confirm that this should be an experimental feature (off by default).

I added some tests. I can add more if anyone has a test case that they want to be sure is covered.

Ok I am going to write some tests now. I updated the gist. Do people like the names "classic" and "html" for the display_style options? Another option would be "classic" and "rich".

~~Oh I was looking at bytes not len.~~ I didn't have .encode('utf8')

I'm still worried about size, but minifying the css and even minifying the html did not make a big difference. It looks like the html_repr for rasm is 38kb .

Ok I think I've addressed all the comments. I just updated the gist with the latest output. Tomorrow I'll start adding tests. The css is now ~300 lines and it gets injected everytime the dataset is rendered. I'm not sure if that is considered too long and if it is I don't quite know what to do about it.

Is there ever a situation in which dimensions would be expandable? It seems like it is hard-coded to collapsed.

I am trying to generalize the css to reduce the length.

I just updated the gist with these changes: - got rid of alternating data colors in preview - fixed icon alignment in row, and rest of data in row - got rid of some not-useful spans - changed from underline to bold for index coords

Yeah the data preview is also shifted vertically.

Yeah I agree that is a strange part of the original PR. The version in the fiddle doesn't do that, so I'd really like to hear back from @benbovy about whether the fiddle css is more uptodate.

I don't really like how the attrs look without the alignment of keys and values. I will think about how to make that more legible

The last fiddle and this PR seem fairly different. Does the fiddle have the most up-to-date hierarchy or is it just somewhere where people were playing around with ideas (in which case I should see what is improved and try to pull those bits of css)?

Ok thanks! I'll get cracking :)

Is there anything that I can do to help get this PR in? Are the items on the TODO list prioritized? One minor comment is that in terms of style for overflow, it might be more legible if the var_names were bolded on hover (fiddle), although that might make them look clickable.

I think it would be good to write the dataset to a netcdf, and read it back in with xarray.open_dataset to ensure everything encodes/decides properly.

Do you mean write a test to do this, or just check it locally?

Yeah I have some time to finish it up. Looking back at it now it seems like masked would be a better kwarg for open_rasterio. Is there anything else that should be changed?

I did as @snowman2 suggested but filled the mask with nans at the end. Since it is now using rasterio directly this should address @fmaussion's concern about nodata (which I think was ending up in the nodatavals attr anyways). I also moved the nodatavals from attrs to encoding when mask==True, but I didn't set it to true by default since I didn't want to break backwards compatibility.

Hmm. Do we really want a np.masked_array or do we just want an array with np.Nans in it? I prefer nans, but I guess the downside is type conversion for ints?

I was wondering if we should just pass the mask option to rasterio like that. I'll try it out.

Is there still interest in implementing the mask option as outlined in this issue?

Having started writing a test, I now think that encoding['source'] is backend specific. Here it is implemented in netcdf4: https://github.com/pydata/xarray/blob/70e9eb8fc834e4aeff42c221c04c9713eb465b8a/xarray/backends/netCDF4_.py#L386 but I don't see it for pynio for instance: https://github.com/pydata/xarray/blob/70e9eb8fc834e4aeff42c221c04c9713eb465b8a/xarray/backends/pynio_.py#L77-L81

Is this something that we want to mandate that backends provide?

Should I add a test that expects .encoding['source'] to ensure its continued presence?

Ah I don't think I understood that adding source to encoding was a new addition. In latest master ('0.11.0+3.g70e9eb8) this works fine:

```python def func(ds): var = next(var for var in ds) return ds.assign(path=ds[var].encoding['source'])

ds = xr.open_mfdataset(['./air_1.nc', './air_2.nc'], concat_dim='path', preprocess=func) ```

I do think it is misleading though that after you've concatenated the data, the encoding['source'] on a concatenated var seems to be the first path.

```python

ds['air'].encoding['source'] '~/air_1.nc' ```

I'll close this one though since there is a clear way to access the filename. Thanks for the tip @jhamman!

A dirty fix would be to add an attribute to each dataset.

I thought @jhamman was suggesting that already exists, but I couldn't find it: https://github.com/pydata/xarray/issues/2550#issuecomment-437157299

Maybe we can inspect the preprocess function like this:

```python

preprocess = lambda a, b: print(a, b) preprocess .code.co_varnames ('a', 'b') ```

This response is ordered, so the first one can always be ds regardless of its name and then we can look for special names (like filename) in the rest.

From this answer: https://stackoverflow.com/a/4051447/4021797

@jhamman that looks pretty good, but I'm not seeing the source in the encoding dict. Is this what you were expecting?

```python def func(ds): var = next(var for var in ds) return ds.assign(path=ds[var].encoding['source'])

xr.open_mfdataset(['./ST4.2018092500.01h', './ST4.2018092501.01h'], engine='pynio', concat_dim='path', preprocess=func) python-traceback

KeyError Traceback (most recent call last) <ipython-input-49-184da62ce353> in <module>() ----> 1 ds = xr.open_mfdataset(['./ST4.2018092500.01h', './ST4.2018092501.01h'], engine='pynio', concat_dim='path', preprocess=func)

/opt/conda/lib/python3.6/site-packages/xarray/backends/api.py in open_mfdataset(paths, chunks, concat_dim, compat, preprocess, engine, lock, data_vars, coords, autoclose, parallel, **kwargs) 612 file_objs = [getattr_(ds, '_file_obj') for ds in datasets] 613 if preprocess is not None: --> 614 datasets = [preprocess(ds) for ds in datasets] 615 616 if parallel:

/opt/conda/lib/python3.6/site-packages/xarray/backends/api.py in <listcomp>(.0) 612 file_objs = [getattr_(ds, '_file_obj') for ds in datasets] 613 if preprocess is not None: --> 614 datasets = [preprocess(ds) for ds in datasets] 615 616 if parallel:

<ipython-input-48-fd450fa1393a> in func(ds) 1 def func(ds): 2 var = next(var for var in ds) ----> 3 return ds.assign(path=ds[var].encoding['source'])

KeyError: 'source' ```

xarray version: '0.11.0+1.g575e97ae'

There is a preprocess argument. You provide a function and it is run on every file.

Yes but the input to that function is just the ds, I couldn't figure out a way to get the filename from within a preprocess function. This is what I was doing to poke around in there:

```python def func(ds): import pdb; pdb.set_trace()

xr.open_mfdataset(['./ST4.2018092500.01h', './ST4.2018092501.01h'], engine='pynio', concat_dim='path', preprocess=func) ```

Is this good to go?

I added some examples on copy for each of the object types and I decided to let Datasets do this too. I just require that the user pass a dict where keys are contained in data_vars and each value has the same shape as the corresponding data_var.

I made the docs changes and added an example.

I think this is good to merge

👍 for xarray.structured_like(new_data, template)

That said. I think with_data is better than anything replace. I would worry about confusion with the pandas replace which replaces specific values.

I don't like it being a method on an existing object. I think the syntax is clearer if the template object is an argument and the second argument is the data. I think that conveys the concept of creating a new object that is just using the structure of another object.

Or what about xr.DataArray.like and xr.Variable.like? My main worry is that it kind of feels like these should return bools. Another option would be xr.DataArray.from_dataarray and xr.Variable.from_variable, but that is maybe also confusing...

Any thoughts on this one?

Great! Thanks so much for all the feedback :)

Thanks for the context!

I can change it. I guess I was looking at Dataset.transpose: https://github.com/pydata/xarray/blob/master/xarray/core/dataset.py#L2498

I chose to use *dims rather than a list of dims so that this change will have a very small impact on people. Most people probably do something like unstack('z') right now, and that will still work.

Ok so in this PR I will make unstack accept multiple dims like xr.DataFrame.unstack(*dims). The order of the dims will only be roundtripped if all dims are stacked into one, but I think that is reasonable.

In a follow on PR I will make xarray.label_like(array, other). I think that notation speaks more to what we are trying to convey, but I do think the position of the arguments isn't intuitive.

we have similar method reset_index. Do we also want to make dim optional?

I don't have an opinion on that except to say that reset_index takes an iter of dims so it is at least slightly different. So to me it seems fine to only make dim optional on unstack.

what should be done if DataArray or Dataset has multiple MultiIndexes. Maybe do we unstack all the MultiIndexes?

I like the idea of unstacking all the MultiIndexes, but in that case should we allow passing in multiple dims? It seems weird to do a recursive unstack in the case of no argument passed without allowing the user to specifically choose multiple dims along which to unstack.

I think it is probably better to just choose a default dim to unstack like this PR does. But maybe it is better to choose the first dim that is MultiIndex rather than the first dim. That way if you do a stack().unstack() you will roundtrip your data since the stacked index gets added to the end of dims. And if you just pass an object with one MultiIndex (probably the most common scenario) unstack will do the right thing. And if you pass an object with multiple MultiIndexes and unstack repeatedly, you will get your original data out.

Are the names time_check and np_check specific enough for core/utils.py?

@shoyer is this something I should be working on? I am happy to, just don't know how this normally goes.

@shoyer that makes sense. @kwilcox, so times should be strings if decode_times=False? Should I do a new pull request?

@kwilcox, I hadn't thought much about this. I guess the intention was to have only python scalars/lists, but I don't know if we want to get in the business of converting attribute values since there are so many options for what they could be. What do you think makes the most sense?

Done!

I made the docs locally and they look how I expect.

Ok, so I merged with the current master and added documentation and a what's-new note. I hope I did that right. @shoyer I am really new to contributing so thanks for all your help. Let me know if anything needs changing.

Ok, I made the little changes. I will start working on the docs.

@shoyer I think I have done all the things that you mentioned and I added DataArray.from_dict().

Ok, I made the changes that you suggested. I still need to work on the from_dict validation to prompt users to give more specific dicts.

Ok, I wrote a Dataset.from_dict class method. I imagine the DataArray one will look pretty similar, so I just wanted to see what you think. The main issue that I see is that time doesn't round trip. I wasn't sure if the user should need to set a parse_date flag with the dim name or something or whether the function should try to convert to time any dim with the string 'time' in it?

``` @classmethod def from_dict(cls, d): """ Convert a dictionary into an xarray.Dataset. """ obj = cls()

    dims=OrderedDict([(k, d['coords'][k]) for k in d['dims']])
    for dim, dim_d in dims.items():
        obj[dim] = (dim_d['dims'], dim_d['data'], dim_d['attrs'])

    for var, var_d in d['data_vars'].items():
        obj[var] = (var_d['dims'], var_d['data'], var_d['attrs'])

    # what it coords aren't dims?
    coords = (set(d['coords'].keys()) - set(d['dims']))
    for coord in coords:
        coord_d = d['coords'][coord]
        obj[coord] = (coord_d['dims'], coord_d['data'], coord_d['attrs'])
    obj = obj.set_coords(coords)

    obj.attrs.update(d['attrs'])

    return(obj)

```

Regarding unit tests, I haven't ever written one before but I would be happy to try (I know they are an important part of good development).

Ok, I have committed a first pass at unit tests. I purposefully wrote a failing time test.

Ok. That makes sense. I added a function to common and submitted a pull request: #917

I agree.

.3. Couldn't this make the dict blow up for large datasets? Maybe there could be a flag that lets the user decide whether to leave the data in its current form (could use self.data in case it is a dask array) 6. The trouble with xarray.DataArray is that it doesn't require a name but it can have one. Is that something that we would want to preserve? If not, then maybe it would look more like this.

``` tolist=True

d = {'coords': {}, 'attrs': dict(self.attrs), 'dims': self.dims}

def func(x, tolist): if tolist: return x.tolist() return x

for k in self.coords: d['coords'].update({k: {'data': func(self[k].data, tolist), 'dims': list(self[k].dims), 'attrs': dict(self[k].attrs)}}) if hasattr(self, 'data_vars'): d.update({'data_vars': {}}) for k in self.data_vars: d['data_vars'].update({k: {'data': func(self[k].data, tolist), 'dims': list(self[k].dims), 'attrs': dict(self[k].attrs)}}) else: d.update({'data': func(self.data, tolist)}) ```