issue_comments

Thanks

I just pushed a commit to your branch that should fix the string identity issue (by not defining the constant multiple times).

I completely forgotten about that little quirk of cpython. I try to ignore implementation details like that. Heck, I still don't fully trust dictionaries to be ordered!

I removed the WIP. We can deal with the concat dim default object separately, including turning it into a ReprObject (not exactly sure what the advantage of it is over just using the string, but, meh).

Is the following statement True or False: "The user should be allowed to explicitly declare that they want the concatenation dimension to be inferred by passing a keyword argument".

This statement is False, but it looks like we have a related bug: we really should be importing _CONCAT_DIM_DEFAULT from xarray.core.combine, because we use identity comparison in that module. Right now this only works by virtue of the accident that identical string literals points to the same variable in CPython.

Is the following statement True or False: "The user should be allowed to explicitly declare that they want the concatenation dimension to be inferred by passing a keyword argument". If this is True, then you need to test equivalence. If it is False, then there is nothing more I need to do for the PR, as changing this to use a ReprObject is orthogonal to these changes.

ok, so we use the ReprObject for the default, and then test if concat_dim is of type `ReprObject and then test its equivalance?

No, just check identity with the exact ReprObject used as the default value.

This is just a slightly more readable version of the common idiom of use object() as a default value, e.g., as shown here http://effbot.org/zone/default-values.htm#what-to-do-instead

ok, so we use the ReprObject for the default, and then test if concat_dim is of type `ReprObject and then test its equivalance?

Yes, it would be better to use the custom ReprObject.

On Fri, Jan 4, 2019 at 1:42 PM Benjamin Root notifications@github.com wrote:

@WeatherGod commented on this pull request.

In xarray/backends/api.py https://github.com/pydata/xarray/pull/2648#discussion_r245428584:

@@ -606,7 +606,7 @@ def open_mfdataset(paths, chunks=None, concat_dim=_CONCAT_DIM_DEFAULT, # Coerce 1D input into ND to maintain backwards-compatible API until API # for N-D combine decided # (see https://github.com/pydata/xarray/pull/2553/#issuecomment-445892746) - if concat_dim is None or concat_dim == _CONCAT_DIM_DEFAULT: + if concat_dim is None or concat_dim is _CONCAT_DIM_DEFAULT:

I guess what I am getting at is a matter of user-friendliness. A user looking up the call signature of this method will see the default as the string (and not an object), but, they'll never be able to explicitly force concat dimension inference, because passing the string themselves will not work (it'll be a different object).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/2648#discussion_r245428584, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1ll3RBImsQ_AzjJbKMdgtHFILXdIks5u_8qwgaJpZM4ZqBxI .