Parallel convolve_to tests and fixes #973
Conversation
…d add spatial parallel smoothing test
| time2 = time.time() | ||
|
|
||
| if time2 - time1 > 0.3: | ||
| if time2 - time1 > 0.5: |
There was a problem hiding this comment.
@astrofrog Any issues with slightly increasing this load time check? It was coming in at ~0.302 s and failing for some of the windows CI tests.
| print(rslt) | ||
|
|
||
|
|
||
| # @pytest.mark.skipif('True') |
There was a problem hiding this comment.
just a reminder - this is probably intended to be removed before merge?
There was a problem hiding this comment.
We had this in before and I wanted to make sure tests would pass when adding these back in.
|
I approved, but the PR at present doesn't resolve the second two checkmarks... I probably should have waited |
Yeah it still needs verification that the new tests are actually testing the parallelized part |
|
The #971 failure may be system specific. It's only failing on the Windows CI tests. |
|
@keflavich -- I don't think we ever have parallel operations in non-dask mode without spectral-cube/spectral_cube/spectral_cube.py Line 3019 in 0b83254 Is that right? |
|
Yes, I see this comment above: spectral-cube/spectral_cube/spectral_cube.py Line 2968 in 0b83254 I think we just need a clear warning and suggestion to use dask in these cases |
|
@keflavich @astrofrog We have some important kwargs relevant to controlling the location of memmap files, etc, hidden in spectral-cube/spectral_cube/spectral_cube.py Line 2891 in 0b83254 Because it's a private method, though, this doesn't get included in the docs. We could either make it a public method (not its intended use) or duplicate some of the kwarg descriptions in |
|
Yeah.... it's kind of 'private' in the sense that it's not meant to be used directly, but the documentation should be public. Maybe we remove the leading underscore, get it into the API docs, but note that "Users should not use this function directly, but the kwargs are used in other functions... [list of functions]" |
|
Agreed. I'll make that change with the warning. |
…thod so it's included in the documentation
…CI that otherwise shouldn't impact normal workflows
2 participants
Addressing #971 and #972
Fix and test parallelization forIssue is not with parallelization from Large cube convolutions - Parallel #971. Same error occurs with other functions and is somehow related to the specific FITS file.convolve_touse_memmap=Falseforconvolve_to-- warning message thatparalleldoes not work with joblib parallelization without memmap. This was noted in the existing code but not passed to the user.big_data.rston selecting the directory for memmap files withmemmap_dir="/path/dir/"disable_huge_flagthat ensures multiple memmaps aren't used in parallel with joblib (based issue in Large cube convolutions - Parallel #971 )