Time allow initialization with bytes or dtype='S' ndarray input by mhvk · Pull Request #6823 · astropy/astropy

mhvk · 2017-11-09T17:29:12Z

Currently, if we pass a Column with dtype='S' to Time, this raises an error even though Column itself knows how to translate the bytes to strings. The problem is that in iterating over the column using np.nditer, the input is treated as a regular array. There are two possible solutions: use the multi_index of the iterator to get the item, so that Column.__getitem__ can return a string, or just being more generally accepting of bytes input, and treating it as ascii. In this PR, I took the latter approach, as it was simpler and I didn't really see a downside to it.

This is helpful for #6821.

astropy-bot · 2017-11-09T17:29:14Z

Hi there @mhvk 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

mhvk · 2017-11-09T18:11:40Z

Failures are the usual URL errors, plus numpy-dev formatting errors unrelated to this PR.

astrofrog · 2017-11-09T18:28:54Z

Hrm I don't get why there are still URL errors. It's really unlikely that both data.astropy.org and the mirror don't work... I guess I'll have to investigate some more.

taldcroft

Quick comments on the code here. Will try to think about this a little more, later.

taldcroft · 2017-11-10T12:09:33Z

                             .format(timestr, self.name))

+    @staticmethod
+    def _decode_if_needed(val, encoding='ascii'):


Right now this encoding kwarg is orphaned, in the sense that it is not actually used and is not exposed in any way to the public API. I'd rather hardwire for now and then add a kwarg IFF it is actually necessary.

taldcroft · 2017-11-10T12:12:33Z

        for val, iy, im, id, ihr, imin, dsec in iterator:
            iy[...], im[...], id[...], ihr[...], imin[...], dsec[...] = (
-                self.parse_string(val.item(), subfmts))
+                self.parse_string(self._decode_if_needed(val.item()), subfmts))


Is there a noticeable performance hit to adding this extra call within the inner loop?

One point is that you know in advance from val1.dtype whether every value needs decoding or none need decoding, so there is some opportunity for optimization. But maybe that is truly in the noise and code clarity is more important.

taldcroft

Overall this is a good thing but has some issues. It seems the whole PR should be re-branded as bringing support for bytes inputs without reference to the column unicode sandwich.

taldcroft · 2017-11-10T13:40:55Z

        for val, iy, im, id, ihr, imin, dsec in iterator:
            iy[...], im[...], id[...], ihr[...], imin[...], dsec[...] = (
-                self.parse_string(val.item(), subfmts))
+                self.parse_string(self._decode_if_needed(val.item()), subfmts))


One point is that you know in advance from val1.dtype whether every value needs decoding or none need decoding, so there is some opportunity for optimization. But maybe that is truly in the noise and code clarity is more important.

taldcroft · 2017-11-10T13:44:00Z

        # Select subformats based on current self.in_subfmt
        subfmts = self._select_subfmts(self.in_subfmt)

+        # We cannot just use the iterator over val1, since that effectively


This comment completely confuses me. Is it a leftover from previous implementation? Maybe below a comment to the effect that the Column unicode sandwich machinery doesn't work here because of how np.nditer works, hence the need to specifically decode each element in the case of a bytestring array val.dtype='S'.

(At least, that is my understanding of the situation..)

[EDIT] - re-reading the PR description it looks like this is a leftover from the multi-index idea.

taldcroft · 2017-11-10T13:58:46Z

    assert (t0 + 1 * u.second) == (t1 + 1 * u.second)
+
+
+def test_string_valued_columns():


Should probably be test_bytes_valued_inputs.

Since the implementation and scope now includes bytes inputs that are not Column, that should be tested.

taldcroft · 2017-11-10T14:00:06Z

+    @staticmethod
+    def _decode_if_needed(val, encoding='ascii'):
+        """Decode ``vall`` if it is a bytes instance."""
+        if type(result) is bytes:


Err, this should be val everywhere not result. This is preventing even import of astropy.

taldcroft · 2017-11-10T14:00:19Z


+    @staticmethod
+    def _decode_if_needed(val, encoding='ascii'):
+        """Decode ``vall`` if it is a bytes instance."""


val not vall

mhvk · 2017-11-10T14:23:42Z

Dang, sorry to have uploaded a version that doesn't even work. With the advent of pytest-astropy, my usual setup didn't work, and hence I didn't test well enough when pushing a fix.

I'm still trying to think what is best, as I don't really like allowing bytes everywhere, but rather would just let Column do its thing. I'll have a quick try to see whether np.vectorize would just do the right thing...

mhvk · 2017-11-10T15:58:59Z

I'm closing thsi in favour of #6832, which I think is better targeted.

taldcroft · 2017-11-10T16:26:57Z

In this case I would tend toward Postel's law of being liberal on input. For instance I might use pytables to read dates from part of one column of a big HDF5 table, then try to feed that straight in to Time. A key point of the column unicode sandwich was to make things "Just Work" where they used to in Py2.

If someone provides a bytes string or array and it parses, then clearly the input was in fact ASCII-encoded and I don't see harm in letting this work. Making this case fail while a bytes Column succeeds doesn't make too much sense to me.

mhvk · 2017-11-10T16:36:16Z

OK, let me make at least make this PR work and then we can compare.

Also add a test for direct bytes input.

mhvk · 2017-11-10T17:13:12Z

@taldcroft - OK, I revisted this one, now no longer using a staticmethod, but just creating a conversion function on the fly. For unicode arrays, this will now be faster than before (since for some reason a.item() is slower than str(a))

mhvk · 2017-11-10T17:57:14Z

circleci failure is unrelated (something with the sites not being up to date).

taldcroft · 2017-11-12T11:34:30Z

@mhvk - excellent! The code change and tests look good to me. The CHANGES.rst entry needs to be updated, and there should be some updates of API / narrative docs to reflect that bytes or dtype='S' are acceptable inputs.

mhvk · 2017-11-12T16:49:15Z

@taldcroft - am not sure we should update documentation - people should really be passing in strings; I see this as a work-around for ndarray subclasses not being dealt with well in nditer, which happens to make us more forgiving, but I don't think we should encourage in any way to input bytes (am very happy to encourage anything else that tries to be a more compactly encoding string like our Column class).

taldcroft · 2017-11-13T02:11:14Z

@mhvk - I guess not updating the narrative docs is OK, but the API docs need to provide accurate information about the API! If the Time object can be initialized with a bytes object then this needs to be documented.

On the bigger picture, I see this as a workaround for the lack of a one-byte string dtype in numpy (remembering that Py2 is no longer part of our conversation). We've made an effort at a workaround with Column unicode sandwich, but there are certainly situations where this is not the right data type for users. So what you've done is great, and extends the astropy workaround for this fundamental numpy problem. We don't need to keep it shamefully hidden under the covers. 😄

mhvk · 2017-11-15T18:40:37Z

@taldcroft - before I had stuff: with "API", do you mean an addition to the Time docstring itself, or to those of the formats (or both)?

taldcroft · 2017-11-15T19:25:57Z

@mhvk - about API docs, from what I can see it is just Time, TimeDelta and TimeFormat. Each has a str entry for val1 that should be supplemented by bytes.

There is a question about what to do for val2. This is documented to accept str but AFAIK in practice it is never used, and is typically just dropped on the floor. So my inclination is to leave that one alone.

mhvk · 2017-11-24T16:45:46Z

@taldcroft - finally got around (end of term...) to wrapping up this PR; please have a look.

p.s. It was amusing to see that some of the docstrings stated "Data to initialize table."

taldcroft · 2017-11-25T16:27:57Z

@mhvk - what is your thinking in marking this as a Bug for backport? I think that the 2.0 behavior is not a really bug since the docs correctly state that only str is allowed, and the default paradigm in Py3 is that str and bytes are not interchangeable. For Py2 + 2.0 LTS, I think this PR makes no difference. So from my perspective this is an API change / usability enhancement to allow a previously-disallowed data type input.

mhvk · 2017-11-25T17:30:09Z

@taldcroft - I think it was a bug that one could not pass in string-valued Column to Time - this used to be possible also in python 3 before you made the sandwich, so is a regression in that sense.

But I don't feel strongly, so if this turns out to be a difficult backport, then I think it is fine to just not do it.

taldcroft · 2017-11-27T11:25:57Z

If there is really a regression then definitely a bug fix. But I cannot reproduce that:

In [1]: astro
astropy=1.3.3

In [2]: Time(Column(['2001:001']))
Out[2]: <Time object: scale='utc' format='yday' value=['2001:001:00:00:00.000']>

In [3]: Time(Column([b'2001:001']))
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-3-df121ead200a> in <module>()
----> 1 Time(Column([b'2001:001']))

ValueError: Input values did not match any of the formats where the format keyword is optional ['datetime', 'iso', 'isot', 'yday', 'fits', 'byear_str', 'jyear_str']

taldcroft · 2017-11-27T11:29:24Z

 - Initialization of Time instances now is consistent for all formats to
  ensure that ``-0.5 <= jd2 < 0.5``. [#6653]

+- Initialization of ``Time`` instances with ``Column`` is now possible


Sorry for missing this, but this should reference the actual change here of generically allowing byte-type input (either bytes or dtype='S' ndarray objects).

In [2]: Time(Column(b'2001:001')) Out[2]: <Time object: scale='utc' format='yday' value=2001:001:00:00:00.000> In [3]: Time(b'2001:001') Out[3]: <Time object: scale='utc' format='yday' value=2001:001:00:00:00.000> In [4]: Time(np.array([b'2001:001'])) Out[4]: <Time object: scale='utc' format='yday' value=['2001:001:00:00:00.000']>

mhvk · 2017-11-27T13:12:43Z

@taldcroft - I think the regression would be that one would have an ascii string column (say, from a fits file) which would be convertible to a Time on 1.3 since it got converted to unicode, but not on 2.0.x because it got sandwiched.

taldcroft · 2017-11-27T14:20:38Z

.. would be convertible to a Time on 1.3 since it got converted to unicode, but not on 2.0.x because it got sandwiched.

Hmm, the table unicode sandwich PR #5700 did not touch FITS or the FITS table reader at all, and left this as a followup #6122. I haven't explicitly verified, but I don't think there is a regression there.

If @astrofrog's #6821 had been part of 2.0 then indeed there could have been a regression (except that he had fixed it internally). Fortunately with this PR all will be well for 3.0!

mhvk · 2017-11-27T14:54:57Z

OK, let's change the milestone to 3.0; I'll update the changelog as well.

Better changelog entry for gh-6823

mhvk added Affects-release Bug time labels Nov 9, 2017

mhvk added this to the v2.0.3 milestone Nov 9, 2017

mhvk requested a review from taldcroft November 9, 2017 17:29

mhvk mentioned this pull request Nov 9, 2017

Make it possible to use Table.read on FITS files with no copying #6821

Merged

taldcroft reviewed Nov 10, 2017

View reviewed changes

taldcroft requested changes Nov 10, 2017

View reviewed changes

mhvk mentioned this pull request Nov 10, 2017

Ensure byte-valued Column inputs to Time are possible. #6832

Closed

mhvk closed this Nov 10, 2017

mhvk reopened this Nov 10, 2017

mhvk added 3 commits November 10, 2017 11:43

Assume ascii encoding for arrays with dtype 'S' in Time.

40d4c61

Add changelog entry.

a231094

Move helper function to TimeString class.

c73764f

Also add a test for direct bytes input.

mhvk force-pushed the time-allow-column-unicode-sandwich branch from 9ab732a to c73764f Compare November 10, 2017 16:44

Ensure string conversion is at least as fast as previously.

885c1b7

mhvk force-pushed the time-allow-column-unicode-sandwich branch from 88ad4b9 to 885c1b7 Compare November 10, 2017 17:11

Update docstrings to reflect that bytes are decoded as ascii [ci skip]

01a0c8c

taldcroft approved these changes Nov 25, 2017

View reviewed changes

taldcroft merged commit 8bdbd10 into astropy:master Nov 25, 2017

mhvk deleted the time-allow-column-unicode-sandwich branch November 25, 2017 17:30

taldcroft reviewed Nov 27, 2017

View reviewed changes

taldcroft changed the title ~~Time allow column unicode sandwich~~ Time allow initialization with bytes or dtype='S' ndarray input Nov 27, 2017

mhvk modified the milestones: v2.0.3, v3.0.0 Nov 27, 2017

mhvk added a commit to mhvk/astropy that referenced this pull request Nov 27, 2017

Better changelog entry for astropygh-6823.

cae7f8a

mhvk added a commit to mhvk/astropy that referenced this pull request Nov 27, 2017

Better changelog entry for astropygh-6823 [ci skip]

14ee13a

mhvk added a commit to mhvk/astropy that referenced this pull request Nov 27, 2017

Better changelog entry for astropygh-6823 [ci skip]

27534ad

bsipocz added a commit that referenced this pull request Nov 27, 2017

Merge pull request #6901 from mhvk/time-column-sandwich-changelog-change

5dec6bb

Better changelog entry for gh-6823

		assert (t0 + 1 * u.second) == (t1 + 1 * u.second)


		def test_string_valued_columns():

Conversation

mhvk commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Nov 9, 2017

Uh oh!

astrofrog commented Nov 9, 2017

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Nov 10, 2017

Uh oh!

mhvk commented Nov 10, 2017

Uh oh!

taldcroft commented Nov 10, 2017

Uh oh!

mhvk commented Nov 10, 2017

Uh oh!

mhvk commented Nov 10, 2017

Uh oh!

mhvk commented Nov 10, 2017

Uh oh!

taldcroft commented Nov 12, 2017

Uh oh!

mhvk commented Nov 12, 2017

Uh oh!

taldcroft commented Nov 13, 2017

Uh oh!

mhvk commented Nov 15, 2017

Uh oh!

taldcroft commented Nov 15, 2017

Uh oh!

mhvk commented Nov 24, 2017

Uh oh!

taldcroft commented Nov 25, 2017

Uh oh!

mhvk commented Nov 25, 2017

Uh oh!

taldcroft commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Nov 27, 2017

Uh oh!

taldcroft commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Nov 27, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mhvk commented Nov 9, 2017 •

edited

Loading

astropy-bot Bot commented Nov 9, 2017 •

edited

Loading

taldcroft commented Nov 27, 2017 •

edited

Loading