I am just curious, I have seen ExitStack being added in this PR and another completely unrelated PR. Is this the new way to do pytest.raises or just context manager in general?

It is no coincidence that you see it suddenly in two places - I saw that completely unrelated PR as well, and thought that it was a neat way to combine context managers, especially if, as here, the number of context managers included is not always the same (without it, I would have needed to use the "null context", which I don't really like - though better than using ExitStack when one has either one or zero context managers).

Shouldn't we aim to make sure the code below doesn't emit a warning?

Oh wait that's due to numpy/numpy#15041 right?

Yes, numpy emits the warning. Since this is just a test that something fails, I think it is not all that important whether there is a warning as well. Eventually, the warning might just change to another exception, but that is OK too.

Can we do this instead with a try/except/catch-warning? Basically try doing val = np.array(val, copy=copy, subok=True) and then if numpy emits a warning or raises an exception then fall through to explicitly specifying dtype=object. That would avoid the special case and hopefully just delegate to numpy whether or not the original val is acceptable.

@taldcroft - coming back to this PR after a while, I remember why I did the upfront test: I'm quite happy to just do try/except for the exception, but changing the warnings filter seems slow and excessive. So, the question then is whether we mind the warnings for the case of [scalar_time, array_time].

p.s. Ideally, to me, the case of an iterable of time would never get turned into an array in the first place...

I did a little profiling, and the impact on initialization of catching warnings is not huge, probably an increase of 2 us on a typical initialization of 65 us. The catching bit could be removed once versions of numpy that issue the deprecation are no longer supported by astropy.

Overall I agree that the current API of allowing a list of ragged Time objects and concatenating is odd and in fact I was surprised myself at that behavior. 😄. But that's a separate decision about whether to deprecate that as well.

I think that allowing a (non-ragged) iterable of Time is OK, certainly given the current behavior:

>>> t = Time([1,2,3], format='cxcsec')
>>> np.array(t)
array([<Time object: scale='tt' format='cxcsec' value=1.0>,
       <Time object: scale='tt' format='cxcsec' value=2.0>,
       <Time object: scale='tt' format='cxcsec' value=3.0>], dtype=object)
>>> Time(np.array(t))
<Time object: scale='tt' format='cxcsec' value=[1. 2. 3.]> 
# Round trips!  

Timing

import warnings
from astropy.time import Time

def fxn(**kwargs):
    x = np.array([1], **kwargs)

def do_fxn():
    with warnings.catch_warnings(record=True) as w:
        try:
            fxn()
        except TypeError:
            fxn(dtype=object)
            
        if len(w) > 0:
            # Check for the automatic object array deprecation warning and ignore,
            # otherwise emit the warning.
            pass

timeit do_fxn()

2.73 µs ± 31.9 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

timeit fxn()

667 ns ± 14.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

timeit Time(50000, format='mjd')

63 µs ± 1.9 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

timeit Time('2019-01-01', format='iso')

66.5 µs ± 1.37 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

That might work, though it means the type of any other warning is lost (only keeps the warning message), and we add yet more that is not thread-safe. The logical route here would be not to record but set up an appropriate filter instead, but that adds another few us, while my if statement is just one line and well under a micro-second.

OK, fair enough. I do think that this kind of special-case opt-in catching is fragile and has some risk of failing for an oddball case you haven't thought about. But I can't decide if that is worse or doing just try/except and exposing annoying warnings that will be obscure to the user. So I'll let you decide. 😄

this may need to be NUMPY_LT_1_19 now, but since everything passed, including the np 1.18 job, maybe it's still OK like this?