Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression fix: if how='slice' or 'ray', we do _not_ want to warn #929

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

keflavich
Copy link
Contributor

#916 introduced a serious regression: warn_slow now completely ignores how='slice' and how='ray', which was not at all the desired behavior.

@keflavich
Copy link
Contributor Author

I could use some feedback here @astrofrog @e-koch : I've removed warnings about dask loading whole cubes into memory, but I'm second-guessing myself now. When you do cube addition, what other option is there than to keep the whole cube in memory?

On the other hand, should we just drop the warnings entirely? I am not sure they're ever appreciated or valuable. This is a bigger question, but for my own use, most of the time I do everything to suppress these warnings...

…getting rid of the test_arithmetic_warning.... why do we care for small cubes? we really don't
@keflavich
Copy link
Contributor Author

This passes completely if I run a local test:

$ pytest spectral_cube
============================================================================================================= test session starts =============================================================================================================
platform darwin -- Python 3.12.3, pytest-8.3.2, pluggy-1.5.0
Running tests in spectral_cube.

Date: 2025-01-06T16:30:52

Platform: macOS-13.6-arm64-i386-64bit

Executable: /Users/adam/mambaforge/envs/py312/bin/python3.12

Full Python Version:
3.12.3 | packaged by conda-forge | (main, Apr 15 2024, 18:35:20) [Clang 16.0.6 ]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions:
Numpy: 2.1.2
Scipy: 1.14.0
Matplotlib: 3.9.2
h5py: 3.11.0
Pandas: 2.2.3
Astropy: 6.1.2
regions: 0.10
APLpy: not available

Using Astropy options: remote_data: none.

rootdir: /Users/adam/repos/spectral-cube
configfile: setup.cfg
plugins: astropy-0.11.0, cov-5.0.0, remotedata-0.4.1, rerunfailures-14.0, filter-subpackage-0.2.0, doctestplus-1.2.1, astropy-header-0.2.2, dependency-0.6.0, anyio-4.4.0, hypothesis-6.111.1, arraydiff-0.6.1, asdf-3.4.0, mock-3.14.0, dash-2.18.2
collected 1721 items

spectral_cube/spectral_axis.py .                                                                                                                                                                                                        [  0%]
spectral_cube/tests/test_analysis_functions.py ...................                                                                                                                                                                      [  1%]
spectral_cube/tests/test_casafuncs.py ......sssssssssssssss                                                                                                                                                                             [  2%]
spectral_cube/tests/test_cube_utils.py .......                                                                                                                                                                                          [  2%]
spectral_cube/tests/test_dask.py .....s.......s.                                                                                                                                                                                        [  3%]
spectral_cube/tests/test_io.py .......................                                                                                                                                                                                  [  4%]
spectral_cube/tests/test_masks.py .....................................................................................XX....                                                                                                           [ 10%]
spectral_cube/tests/test_moments.py ............................................................................................................................................................................................        [ 21%]
spectral_cube/tests/test_performance.py ...s..                                                                                                                                                                                          [ 21%]
spectral_cube/tests/test_projection.py .......................x..............xxx.......x..........................................................................                                                                      [ 28%]
spectral_cube/tests/test_regrid.py ........................................................                                                                                                                                             [ 31%]
spectral_cube/tests/test_spectral_axis.py ....................................................                                                                                                                                          [ 34%]
spectral_cube/tests/test_spectral_cube.py ............................................................................................................................................................................................. [ 45%]
.......................................................................................s.....s...........sssss.................................................ssssss......................ss.......................................... [ 59%]
................................................................................ssssss......ssssssssssss......ssssssssssss......ssssssssssss......ssssssssssss......ssssss............................................................. [ 72%]
.............................ssssss......ssssssssssss......ssssssssssss......ssssssssssss......ssssssssssss......ssssss.................................s.s..............xxxx.......................................................... [ 86%]
..........ssss............................xxxxxxxx.........................................                                                                                                                                             [ 91%]
spectral_cube/tests/test_stokes_spectral_cube.py ....................................................................................................                                                                                   [ 97%]
spectral_cube/tests/test_subcubes.py ......................                                                                                                                                                                             [ 98%]
spectral_cube/tests/test_visualization.py ....ssss..ss                                                                                                                                                                                  [ 99%]
spectral_cube/tests/test_wcs_utils.py ............                                                                                                                                                                                      [100%]

==================================================================================== 1537 passed, 165 skipped, 17 xfailed, 2 xpassed in 287.50s (0:04:47) ====================================================================================

but I reproduce the exact failure with tox. I don't understand it.

@e-koch
Copy link
Contributor

e-koch commented Jan 6, 2025

For the cube addition: are we defaulting to a new memory map or zarr for the dask versions? If so, I don't see a particular issue with dropping the warnings.

@keflavich
Copy link
Contributor Author

regarding the casa reading failures: I'm not sure the above is a 'real life' bug and not just something weird about the tox system.

@keflavich
Copy link
Contributor Author

cube addition: I think it's a new in-memory array. But if we default to a new memmap, I'd be happier with that.... is that a thing we can do? We have to tell numpy to do it...

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 79.79%. Comparing base (4d7182f) to head (585022b).
Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
spectral_cube/io/casa_image.py 0.00% 6 Missing ⚠️
spectral_cube/dask_spectral_cube.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
- Coverage   79.84%   79.79%   -0.06%     
==========================================
  Files          24       23       -1     
  Lines        6024     6037      +13     
==========================================
+ Hits         4810     4817       +7     
- Misses       1214     1220       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@keflavich
Copy link
Contributor Author

Green on all but the CASA tests, for which it seems a STOKES keyword has gone missing. @preshanth, could you have a look?

@keflavich
Copy link
Contributor Author

did a little investigating; it looks like this is a real issue, and we should check whether stokes1 is present, and if not, assume I. I'll leave that to @preshanth to verify though

@preshanth
Copy link
Collaborator

The issue seems to be that the cubes being used in the test have no stokes axis. These are perfectly valid cubes. You can see from the desc header values here is the one where the tests are failing.
['telescope', 'observer', 'obsdate', 'pointingcenter', 'telescopeposition', 'direction0', 'worldmap0', 'worldreplace0', 'pixelmap0', 'pixelreplace0', 'spectral1', 'worldmap1', 'worldreplace1', 'pixelmap1', 'pixelreplace1']
We are searching for stokes1. The way casa arranges the headers is it checks for direction, then stokes and then spectral. If there is no stokes found we can default to Stokes I and throw a warning and proceed. That would take care of this issue.

@preshanth
Copy link
Collaborator

I did this and it gets over the error altogether.

        # check if stokes1 or stokes 2 is present. if not assume stokes_params = ['I']
        if 'stokes1' in desc['_keywords_']['coords']:
            stokes_params = desc['_keywords_']['coords']['stokes1']['stokes']
        elif 'stokes2' in desc['_keywords_']['coords']:
            stokes_params = desc['_keywords_']['coords']['stokes2']['stokes']
        else:
            # throw a warning
            warnings.warn("No stokes information found in CASA image. Assuming Stokes I.")
            stokes_params = ['I']```
This should cover most cubes unless people come up with a really crazy stokes cube. I will try and cook one of those up in the meantime. I have to see what the fits standard allows.

@keflavich keflavich requested a review from e-koch January 8, 2025 18:50
Copy link
Contributor

@e-koch e-koch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One minor change to get rid of a couple print statements before merging.

@@ -107,8 +107,14 @@ def load_casa_image(filename, skipdata=False, memmap=True,
print(desc['_keywords_']['coords'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove temp print statements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants