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

Ensure that MaskedColumn.data returns a plain MaskedArray. #8855

Merged
merged 1 commit into from Jun 30, 2019

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Jun 15, 2019

Previously this was instead a masked_BaseColumn, which caused a number of performance hits, e.g., because slicing would go through multiple rounds of __array_finalize__. This PR includes a regression check for that count.

fixes #6721

@codecov
Copy link

codecov bot commented Jun 15, 2019

Codecov Report

Merging #8855 into master will increase coverage by 0.4%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #8855     +/-   ##
=========================================
+ Coverage   86.59%   86.99%   +0.4%     
=========================================
  Files         405      400      -5     
  Lines       60116    59480    -636     
  Branches     1100     1100             
=========================================
- Hits        52056    51745    -311     
+ Misses       7419     7094    -325     
  Partials      641      641
Impacted Files Coverage Δ
astropy/table/column.py 95.12% <100%> (-0.02%) ⬇️
astropy/visualization/wcsaxes/coordinates_map.py 86.88% <0%> (-10.62%) ⬇️
astropy/constants/__init__.py 94.87% <0%> (-5.13%) ⬇️
astropy/io/misc/pickle_helpers.py 97.77% <0%> (-2.23%) ⬇️
astropy/visualization/wcsaxes/utils.py 99.01% <0%> (-0.99%) ⬇️
astropy/visualization/wcsaxes/transforms.py 90% <0%> (-0.91%) ⬇️
astropy/io/fits/convenience.py 85.76% <0%> (-0.8%) ⬇️
astropy/utils/iers/iers.py 92.16% <0%> (-0.78%) ⬇️
astropy/stats/bayesian_blocks.py 95.3% <0%> (-0.68%) ⬇️
astropy/coordinates/earth.py 75.84% <0%> (-0.38%) ⬇️
... and 66 more

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 a0e7363...5f99eef. Read the comment docs.

Previously this was instead a masked_BaseColumn, which caused a number
of performance hits, e.g., because slicing would go through multiple
rounds of ``__array_finalize__``. This includes a regression check for
that count.
@mhvk mhvk force-pushed the maskedcolumn-data-property branch from 0272ae6 to 5f99eef Compare June 30, 2019 01:49
@mhvk
Copy link
Contributor Author

mhvk commented Jun 30, 2019

@taldcroft - as you're upping the performance of Table, a reminder of this smaller contribution that fixes a bug in MaskedColumn and greatly reduces the number of __array_finalize__ calls.

@taldcroft
Copy link
Member

@mhvk - sorry for the delay, this looks great! Only thing is that the 32-bit fails look real, mostly related to Table in some way, but I can't quite connect the dots.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 30, 2019

32-bit is fortunately not connected - see #8934

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

OK, ignoring the 32-bit fail.

@taldcroft taldcroft merged commit d3e9b50 into astropy:master Jun 30, 2019
@mhvk mhvk deleted the maskedcolumn-data-property branch June 30, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How many times does MaskedColumn need to be finalized?
2 participants