Repository navigation
Added a utility function to get a non-scalar SkyCoord from a list of targets - #215
StuartLittlefair wants to merge 2 commits into
Conversation
|
These tests will currently fail since the tests in |
|
I'd like to resurrect this PR in light of the broadcasting issues with astropy 1.3 (#282, #263). This PR should now pass tests, and moving away from using a list of FixedTarget objects to a vector SkyCoord for internal calculations may be the easiest way to deal with the broadcasting issues, and would probably speed up the code and make it more powerful. |
aa490e5 to
06de1e3
Compare
|
Thanks @StuartLittlefair. I'll pick this PR into my testing branch, that contain all the fixes with the other issues. It that pass we can have a big merging event :) |
|
@bsipocz the approach I'm suggesting will probably involve a large amount of re-factoring in the end, but I'm thinking we should always be broadcasting the targets (first axis) against the obstimes, which can be any shape. An example would be that the current if not isinstance(time, Time):
time = Time(time)
if time.isscalar:
time = Time[np.newaxis]
altaz_frame = AltAz(location=self.location, obstime=time,
pressure=self.pressure, obswl=obswl,
temperature=self.temperature,
relative_humidity=self.relative_humidity)
if target is None:
# Return just the frame
return altaz_frame
else:
# convert target argument to vector SkyCoord
targets = get_skycoord(target)
if targets.isscalar:
targets = targets[np.newaxis]
# now we broadcast the targets array so that the
# first index iterates over targets, any remaining indices over times
while targets.ndim <= altaz_frame.ndim:
targets = targets[:, np.newaxis]
return targets.transform_to(altaz_frame) |
|
@StuartLittlefair - The PR is it's current form is merged into a test branch that includes the other fixes, too, but there are still a few broadcasting failures both in the code and the docs, e.g. While the proposed behaviour changes looks OK to me, I have to state that I don't know the code itself well enough to make any decisions. |
|
Hi @bsipocz Thanks for including the pr. It won't, by itself fix the broadcasting issues but the helper function it provides might give a way to work with astropy coordinate broadcasting rather than against it. I've been working on a PR based on the current master to show how this might be done, but it's not a quick fix and due to other time pressures probably won't be available to address #282 in a timely fashion unless someone else picks it up. |
|
@StuartLittlefair - sure, no worries, I was just mislead by the comment above that this should pass the tests. |
|
Superseded by #285 |
The rationale behind this PR is that it is often faster to make calculations with a non-scalar SkyCoord than with a list of
FixedTargetobjects orSkyCoordobjects. However, making the conversion is non-trivial so a helper function is required.The utility function described here will return a
SkyCoordobject when passed either:FixedTargetorSkyCoordFixedTargets orSkyCoords.In the latter case, the returned
SkyCoordwill be non-scalar.In the case where all of the supplied target coordinates are in the same frame, no conversion will take place. If there is a mix of frames, all coordinates will be converted to
ICRS.One potentially controversial decision is what to do when there is a mix of coordinates with distances and those without. You cannot create a
SkyCoordwith a mix like this. It is, IMO, not right to discard the distances from targets that have them, since any transformation into a frame with a different origin will be incorrect. Instead, I assign an arbitrary, large, distance to those targets which lack it.