Healpix mapmaker #179
Healpix mapmaker #179
Conversation
Case where target ndim smaller than shape-check tuple
This adds flexibility and is needed to match TOAST qpoint Note we do no checking to avoid duplicates with default args (accuracy, fast_math, mean_aber, rate_ref) or those from weather.to_qpoint() (temperature, humidity, pressure)
Remove dummy axis and add RING-NEST option for untiled healpix maps Add error checking for NSIDE
Comment out print statements in projection
There was a problem hiding this comment.
Sorry it took me a while to get to this. This is appreciated!
A few comments inline. Also:
- The demo is helpful, but there should be unit tests too (in test/). (See test_proj_eng.py ... I think it's best if unit tests run quickly, i.e. using very small problems for the trials.)
- Add some basic instructions to the docs (doesn't have to be exhaustive; just a starting point / example).
src/Projection.cxx
Outdated
| @@ -1146,7 +1426,6 @@ bp::object ProjectionEngine<C,P,S>::pixel_ranges( | |||
| target_ranges[i_det].append_interval_no_check(domain_start, n_time); | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Please restore these lines ... unnecessary reformatting of untouched code is to be avoided.
|
Thanks for the detailed comments! With respect to the docs, my restructuring of the |
Fix from_map and _guess_comps Fix RING Change order of optional arguments for for_healpix constructor Refactor get_active_tiles
|
I believe these commits address all of the above points, except for the docs issue I mentioned in my previous comment |
There was a problem hiding this comment.
Thanks for the additional fixes. This is almost ready.
For the Projectionist docs, I think you just need to put all 3 classes in the autodocs, and then create an explicit link in the docstring for the two subclasses saying "see also the methods defined in base class, ...". I think it's dangerous and annoying to duplicate docstring text ... but helpful to provide clickable links in the rendered documentation to the one source of truth. (This approach is already in there, where method docstrings don't all explain what is meant by arguments "assembly", "comps", etc ... but rather that's explained in the class docstring. All that stuff in the Projectionist docstring should probably go in the base class docstring, then, and just a link remains (along with everything specific to RectPix).)
Change default method to 'simple' for untiled maps and 'tiles' for tiled Specific error if method not allowed for Healpix (ie domdir)
python/proj/wcs.py
Outdated
| """ Construct a Projectionist for Healpix maps. | ||
|
|
||
| See class documentation for description of standard arguments. | ||
|
|
There was a problem hiding this comment.
Please make formatting of docstring a little more consistent with the rest of the module... e.g.:
"""Construct a Projectionist for Healpix maps.
See class documentation for description of standard arguments.
(No space after """; paragraph edges are in the column of the first ".)
| def get_coords(self, assembly, use_native=False, output=None): | ||
| """Get the spherical coordinates for the provided pointing Assembly. | ||
| See parent class documentation for full description. |
There was a problem hiding this comment.
It's not really necessary to redefine this method, right? There's nothing proj specific about get_coords. I know the default version uses CAR, but that's true, under the hood, for the HP projector too.
So... please remove this, and just update the inherited get_coords docstring if you feel it is not general enough.
There was a problem hiding this comment.
Agree it's not needed in principle. As written there is a technical problem, in that the inherited get_coords will try to initialize a CAR ProjEng using the Healpix _get_pixelizor_args info (nside, etc) and crash.
I can add a small check to the parent function to call the HP rather than CAR ProjEng, or there are other solutions (eg add a default arg to call 'CAR' with a minimal re-definition in the Healpix case to call 'HP', or a bit more work in the Healpix case to make a proper CAR Projectionist), or else we can leave it as is. Do you have a preference?
There was a problem hiding this comment.
Ah, right. Ok, that's a good enough reason. Leave it in.
|
btw @earosenberg Please squash this on merge (or before). |
No description provided.