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

Added stokes_data public method #751

Merged
merged 14 commits into from
Nov 3, 2021

Conversation

Kitchi
Copy link
Collaborator

@Kitchi Kitchi commented Oct 5, 2021

It was previously not possible to cleanly access the underlying data of
a StokesSpectralCube, which only had the _stokes_data method. This
commit implements a simple StokesSpectralCube.stokes_data method for
public access.

This implements the first suggestion in #750

It was previously not possible to cleanly access the underlying data of
a `StokesSpectralCube`, which only had the `_stokes_data` method. This
commit implements a simple `StokesSpectralCube.stokes_data` method for
public access.
Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

@e-koch @astrofrog any reason not to do this?

For some reason, reshaping the array then running map_blocks leads to memory issues when dealing with Numpy memory mapped arrays, e.g. with FITS files. This new approach avoids reshaping the array at all.
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #751 (daababa) into master (320d9c4) will increase coverage by 0.01%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
+ Coverage   77.55%   77.57%   +0.01%     
==========================================
  Files          24       24              
  Lines        5775     5783       +8     
==========================================
+ Hits         4479     4486       +7     
- Misses       1296     1297       +1     
Impacted Files Coverage Δ
spectral_cube/stokes_spectral_cube.py 86.20% <53.84%> (-5.80%) ⬇️
spectral_cube/dask_spectral_cube.py 85.07% <0.00%> (-0.11%) ⬇️
spectral_cube/spectral_cube.py 76.77% <0.00%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 320d9c4...daababa. Read the comment docs.

keflavich and others added 4 commits October 21, 2021 19:04
Avoid memory issues when computing statistics
…endian data and provides no performance improvements
As per the second suggestion in radio-astro-tools#750 - the Stokes data can now be
accessed via cube['I'], cube['Q'] etc.

Assigning new keys is not yet supported and will throw
NotImplementedError if attempted.
@keflavich
Copy link
Contributor

I'm fine with this; will merge soon if there are no objections

@e-koch
Copy link
Contributor

e-koch commented Nov 3, 2021

@Kitchi or @preshanth -- The tests are now fixed from #759. Can you rebase this branch?

@preshanth
Copy link
Collaborator

I just did @e-koch . Hope that is enough

Kitchi and others added 4 commits November 3, 2021 14:27
It was previously not possible to cleanly access the underlying data of
a `StokesSpectralCube`, which only had the `_stokes_data` method. This
commit implements a simple `StokesSpectralCube.stokes_data` method for
public access.
As per the second suggestion in radio-astro-tools#750 - the Stokes data can now be
accessed via cube['I'], cube['Q'] etc.

Assigning new keys is not yet supported and will throw
NotImplementedError if attempted.
@e-koch
Copy link
Contributor

e-koch commented Nov 3, 2021

Windows test failure is unrelated and known from #759

@e-koch e-koch merged commit 27896c6 into radio-astro-tools:master Nov 3, 2021
@Kitchi Kitchi deleted the stokes_methods branch November 4, 2021 06:27
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.

6 participants