issue_comments

Unit Test Results

6 files  ±0           6 suites  ±0   57m 49s :stopwatch: ±0s 16 227 tests ±0  14 492 :heavy_check_mark: ±0  1 735 :zzz: ±0  0 :x: ±0  90 558 runs  ±0  82 384 :heavy_check_mark: ±0  8 174 :zzz: ±0  0 :x: ±0 

Results for commit 43409090. ± Comparison against base commit 43409090.

:recycle: This comment has been updated with latest results.

Thanks @lusewell and @spencerkclark

Unfortunately we don't do backports.

Fixed other usages and added to whats-new.rst

Thanks for catching that additional spot.

Its only a performance issue to attempt to import cftime repeatedly. Having it fail once in the top level import is not a big problem.

Yes, understood. I just prefer that we are consistent across the code base -- either we use this pattern only where absolutely necessary or we use it everywhere. In light of that do you mind introducing this pattern in times.py as well? There are two places where cftime is imported within functions there too:

https://github.com/pydata/xarray/blob/7bfee3eaa8fd731494cf6b406d6abb4bec061001/xarray/coding/times.py#L167

https://github.com/pydata/xarray/blob/7bfee3eaa8fd731494cf6b406d6abb4bec061001/xarray/coding/times.py#L417

I think we don't have to worry about the tests, because they already follow this pattern to an extent; in building the requires_cftime decorator, a cftime import is only attempted once.

After that, just fix the linting error and add a what's new entry, and I think this should be ready to go from my perspective.

@spencerkclark

RE perfomance.

Its only a performance issue to attempt to import cftime repeatedly. Having it fail once in the top level import is not a big problem. The issue comes when it does it thousands of times every time you try and .sel or .isel, which then adds up to a huge performance hit. Given xarray takes a while to import anyway, the marginal cost of search ing the full pythonpath 3 times in import is minimal - only an issue when done repeatedly.

I've fixed this for some other cases I've found that were causing me slowness - would like me to changeanythinng else before this can be merged?

So i've found another instance of this which causes a performance issue - this one with groupby.

doesn't look like it?

But in both cases we always check for the existence of cftime via import cftime, so if python is clever enough to remember that cftime doesn't exist the second time it's asked to import it, then where is the opportunity for speedup?

Hopefully @lusewell can enlighten us :sweat_smile:

I guess it always tries importing if the module doesn't exist and so that's a slowdown?

Thanks for the suggestion @lusewell .

I'm a bit confused as to how exactly this improves performance though - you've moved the location of the import cftime statement to the top of the file, but I was under the impression that python doesn't ever import a module more than once, because after the first time it's a fast hash lookup. So surely in both cases we only look for the existence of cftime once? Perhaps I've misunderstood though?

I'd also like to append this to tag 14.1 and make tag 14.2 if possible - would this be ok?