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

Support for boolean/mask maps #49

Merged
merged 62 commits into from
Oct 4, 2022
Merged

Support for boolean/mask maps #49

merged 62 commits into from
Oct 4, 2022

Conversation

nwhitehorn
Copy link
Member

@nwhitehorn nwhitehorn commented Apr 30, 2021

It is often useful to have boolean sky maps (e.g. a mask) which are 64x larger than they strictly need to be if represented as doubles. This provides support for such a thing by providing a new sparse storage backend for maps based on std::vector, which stores entries as single bits. Semantics are the following:

  • Comparison operators return a boolean map
  • Multiplication of a map by a boolean map is implemented efficiently

To Do:

  • Sparse backend for healpix masks
  • Comparison operators as members of G3SkyMap (use boost::shared_ptr)
  • Efficient mask multiplication operators for FlatSkyMap and HealpixSkyMap
  • Tests
  • Use masks everywhere
  • Create masks from numpy arrays
  • Convert mask to numpy array
  • Support numpy ufunc interface for all() and friends, at least in the simple case
  • Documentation

@arahlin arahlin self-requested a review July 12, 2022 14:21
@arahlin
Copy link
Member

arahlin commented Jul 30, 2022

Ran into a bug accessing the mask parent attribute:

In [1]: from spt3g import core, maps

In [2]: x = maps.FlatSkyMap(300, 300, core.G3Units.arcmin)

In [3]: m = x.to_mask()

In [4]: m.parent
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [4], in <module>
----> 1 m.parent

TypeError: No to_python (by-value) converter found for C++ type: boost::shared_ptr<G3SkyMap const>

@arahlin
Copy link
Member

arahlin commented Jul 31, 2022

This PR is now feature-complete, up to the bug above, and an API converting masks to/from numpy arrays. It would be useful to add in the sparse backend handling for healpix maps with this PR, too, otherwise this won't do much in terms of memory efficiency for healpix maps.

@arahlin arahlin marked this pull request as ready for review August 3, 2022 04:57
@arahlin
Copy link
Member

arahlin commented Aug 5, 2022

There are many instances of the numpy ufunc reduction wrappers (e.g. np.all()) being used for all, any or sum on sky maps or masks. These now fail because the corresponding C++ methods don't have the signature that the numpy ufunc machinery expects. I think the right solution is to add wrapper functions to skymapaddons.py that allow only the simple case (where the input arguments are axis=None, out=None) and otherwise raise a useful error. I might also consider adding similar functionality for mean and std, since these are also commonly used, so we can avoid many instances of producing accidentally dense maps. The alternative is removing the C++ functions and letting the numpy accessors make the maps dense in order to use its internal ufunc machinery, and I'd rather avoid that if possible. Thoughts?

@nwhitehorn
Copy link
Member Author

I think the right solution is to add wrapper functions to skymapaddons.py that allow only the simple case (where the input arguments are axis=None, out=None) and otherwise raise a useful error.

This is the way. Anything else will be a lot of work and still be fragile.

Copy link
Member Author

@nwhitehorn nwhitehorn left a comment

Choose a reason for hiding this comment

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

Sasha's changes look good to me. I can't actually press "approve" because I apparently wrote too much of the code in the PR but I approved all the same.

@arahlin arahlin merged commit d38767d into master Oct 4, 2022
@arahlin arahlin deleted the booleanmaps branch October 4, 2022 17:56
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.

2 participants