APE 14 implementation #146

nden · 2018-03-26T13:11:50Z

APE 14 implementation.

coveralls · 2018-03-26T13:21:13Z

Coverage increased (+1.05%) to 89.125% when pulling 4ef54e5 on nden:ape14 into 9c7d371 on spacetelescope:master.

nden · 2018-03-26T13:46:18Z

@astrofrog @Cadair This is a start of APE14 implementation. It's pretty straightforward to implement in gwcs though some questions came up, specifically the top three in the description above may need clarification.

Cadair · 2018-03-26T14:14:25Z

gwcs/wcs.py

+        return is_separable(self.forward_transform)
+
+    def pixel_to_world_values(self, *pixel_arrays):
+        # need **kwargs


I am not convinced we do need this here, this API doesn't need to encapsulate the whole of the gWCS functionality.

Having said that I am not sure there is a problem with having it in the function signature. (Other than output should be prohibited)

There's a suggestion that the result should return NaNs where there's no valid WCS solution. This is controlled by keywords and their current defaults provide this functionality. So yes, currently it's not strictly needed.

astrofrog · 2018-03-26T15:44:35Z

@nden - just for info, I've started a document here with issues I've encountered while implementing this on the astropy side: [will send link in slack] feel free to make suggested edits if you have any further thoughts! Once we've both had a chance to think about these issues and try and push the implementation further, my plan is to send this out to the main APE authors and then if there is agreement, to open a PR to amend the APE.

Cadair · 2018-08-13T15:28:56Z

gwcs/wcs.py

+    @property
+    def axis_correlation_matrix(self):
+        from astropy.modeling import separable
+        return separable.is_separable(self.forward_transform)


I dont think this is correct, the output should be a matrix. The closes I could cook up is:

s = separable.is_separable(spatial & Identity(1)) sep = np.identity(len(s), dtype=bool) for i, a in enumerate(s): sep[i,i] = not(a)

Right. I will have to work on this. It isn't ready for review yet, especially thie property.
that said comment would be very useful.

sure, I actually came here looking for how you worked this out for compoundmodels in general and ended up getting a little distracted by matrices lol

astrofrog · 2018-09-28T13:16:09Z

Note that astropy/astropy#7326 has now been updated and is 'ready' (apart from more tests that are needed)

order of axes in pixel_shape and pixel_bounds

This is now clarified in the APE. Note that pixel_shape is now array_shape

how to pass kwargs to pixel_to_world_values and world_to_pixel_values

I don't think we allow this for now. What use case did you have in mind?

implement world_axis_object_classes - perhaps don't require classes but a callable?

I think you can now simply implement world_to_pixel and pixel_to_world yourself and just raise a NotImplementedError for world_axis_object_classes and world_axis_object_components - this was what you were planning on doing I think?

nden · 2018-09-28T14:19:12Z

how to pass kwargs to pixel_to_world_values and world_to_pixel_values

I don't think we allow this for now. What use case did you have in mind?

The use case is passing non-coordinate inputs, like spectral_order or slit_name.
The general implementation of this still needs to be worked out in gwcs.
Specific examples exist in jwst.

I think you can now simply implement world_to_pixel and pixel_to_world yourself and just raise a
NotImplementedError for world_axis_object_classes and world_axis_object_components - this was
what you were planning on doing I think?

Yes, this is fine.

astrofrog · 2018-09-28T18:37:46Z

@nden - is there any reason why you couldn't essentially treat slit_name or spectral_order as keys to a dictionary of WCS objects? That is: overall_wcs[slit_name][spectral_order].world_to_pixel(...). Having said that, I'm not completely opposed to the kwargs, but I just want to make sure I understand.

nden · 2018-09-28T18:51:23Z

@astrofrog I could but it's a dict then and not a WCS object. Many of the JWST spectral pipelines are setup as one WCS object. For example an IFU observation has one WCS for all slices. The reason is that many of the transforms are the same and are shared among slices.
Another example is the slitless spectroscopy where the models are global and are parametrized by spectral order and locaiton (IIRC).

astrofrog · 2018-09-28T20:23:02Z

@nden - actually is there any reason why you can't choose to add kwargs in this implementation since you are going to implement both the low- and high-level API? In other words, I guess the APE doesn't forbid you from doing this?

nden · 2018-09-30T05:54:45Z

@astrofrog Yes, I will add **kwargs in this implementation. I just want to make sure that the option is available in the top level interface used in nddata (if my understanding of how this is going to work is correct).

Cadair · 2018-10-10T15:20:18Z

gwcs/api.py

+        """
+        if not self.forward_transform.uses_quantity:
+            kwargs = {'with_units': True}
+        return self.__call__(*pixel_arrays, **kwargs)


why the __call__?

Yes, strictly speaking, it's not necessary.

Cadair · 2018-10-10T15:21:54Z

gwcs/api.py

+        return False
+
+    def world_axis_object_classes(self):
+        raise NotImplementedError()


Is the plan to implement these to facilitate a common high level API object? (In addition to gWCS implementing the high level api?)

No, I wasn't planning on implementing it. Is there a need for this?

astrofrog · 2018-11-01T12:19:55Z

@nden - can you try updating to the latest developer version of astropy to make sure that things work properly now?

nden · 2018-11-01T15:25:10Z

@astrofrog Done and tests passing.

I'd like to merge this in about a week at the latest. If anyone has the time to review please do so before then. cc / the APE authors: @Cadair @eteq

nden · 2018-11-07T22:08:02Z

Last ping if anyone has time to review or intends to review it and needs more time.

nden added the enhancement label Mar 26, 2018

Cadair reviewed Mar 26, 2018

View reviewed changes

pllim mentioned this pull request Mar 27, 2018

Add option to use gWCS ejeschke/ginga#327

Closed

This was referenced Jun 6, 2018

[WIP] Implement ASDF IO for Ginga spacetelescope/stginga#157

Closed

WIP: Ginga viewer for ASDF spacetelescope/jwst-scsb#3

Closed

nden force-pushed the ape14 branch from c262e50 to 2b1c096 Compare June 21, 2018 20:20

nden modified the milestones: 1.0, 0.9 Jun 21, 2018

nden force-pushed the ape14 branch from e99e3a4 to 5680d7b Compare August 2, 2018 20:46

Cadair reviewed Aug 13, 2018

View reviewed changes

nden force-pushed the ape14 branch from 5680d7b to f6501ff Compare September 6, 2018 11:52

nden force-pushed the ape14 branch from 522e973 to 74f0ce2 Compare September 29, 2018 19:36

nden modified the milestones: 0.9, 0.10 Sep 30, 2018

nden changed the title ~~WIP: APE 14~~ APE 14 implementation Oct 1, 2018

nden added the ready for final review label Oct 1, 2018

Cadair reviewed Oct 10, 2018

View reviewed changes

ape 14 implementaiton

4ef54e5

nden force-pushed the ape14 branch from 1f5a2b6 to 4ef54e5 Compare November 1, 2018 16:36

nden merged commit e59eb05 into spacetelescope:master Nov 8, 2018

nden deleted the ape14 branch December 27, 2018 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APE 14 implementation #146

APE 14 implementation #146

nden commented Mar 26, 2018 •

edited

Loading

coveralls commented Mar 26, 2018 •

edited

Loading

nden commented Mar 26, 2018

Cadair Mar 26, 2018

nden Mar 26, 2018

astrofrog commented Mar 26, 2018 •

edited

Loading

Cadair Aug 13, 2018 •

edited

Loading

nden Aug 13, 2018

Cadair Aug 13, 2018

astrofrog commented Sep 28, 2018 •

edited

Loading

nden commented Sep 28, 2018

astrofrog commented Sep 28, 2018

nden commented Sep 28, 2018

astrofrog commented Sep 28, 2018 •

edited

Loading

nden commented Sep 30, 2018

Cadair Oct 10, 2018

nden Oct 10, 2018

Cadair Oct 10, 2018

nden Oct 10, 2018

astrofrog commented Nov 1, 2018

nden commented Nov 1, 2018

nden commented Nov 7, 2018

APE 14 implementation #146

APE 14 implementation #146

Conversation

nden commented Mar 26, 2018 • edited Loading

coveralls commented Mar 26, 2018 • edited Loading

nden commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Mar 26, 2018 • edited Loading

Cadair Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Sep 28, 2018 • edited Loading

nden commented Sep 28, 2018

astrofrog commented Sep 28, 2018

nden commented Sep 28, 2018

astrofrog commented Sep 28, 2018 • edited Loading

nden commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Nov 1, 2018

nden commented Nov 1, 2018

nden commented Nov 7, 2018

nden commented Mar 26, 2018 •

edited

Loading

coveralls commented Mar 26, 2018 •

edited

Loading

astrofrog commented Mar 26, 2018 •

edited

Loading

Cadair Aug 13, 2018 •

edited

Loading

astrofrog commented Sep 28, 2018 •

edited

Loading

astrofrog commented Sep 28, 2018 •

edited

Loading