UPDATE 1: Fixed a few minor issues causing test failures.
UPDATE 2: All tests pass.
@jzuhone should have added you as a reviewer, because this might help with getting better axis names for the FITS frontend.
All tests pass. Good job!
Note that, at present, this does not do the right thing for spherical and cylindrical coordinate system naming. In fact, there is a slight regression -- see attached, where I have sliced along phi in a spherical dataset.
Looks like a step in the right direction. Glad to see this is being addressed!
Do you think it makes sense to have helper functions for pulling the x_axis and y_axis from the user, just to make sure the input is sanitized before it messes things up? Like:
or something like that? It may be a way of cleaning up x_axis and y_axis too, in that one only has to specify a 3-tuple instead of a 6-tuple. It seems more intuitive that way, but maybe I'm not grasping the whole problem and we're missing information if we go for this method.
Good work, @MatthewTurk .
Hi Cameron, I'm not sure I understand what you're suggesting? How would I use this function?
Will a user need to specify the full axis information each time as a 6-tuple as in your script?
I was simply suggesting that rather than passing it a dictionary to the coordinates structure, you have it build the coordinates from a function that only accepts 3 values so there is less room for a mixup (and it seems like the second set of coords are degenerate):
ds.coordinates.set_xaxis(1,2,0) or ds.coordinates.set_xaxis(x=1,y=2,z=0)
Or were you planning to have default axis information that is hidden away from the user and never needs to be manually set, and you're only exposing it here for the purposes of this PR?
I exposed it here - this is resetting internal variables for the purpose of
demonstration. It's not supposed to.be user-facing.
OK, then. Looks good.
I think as long as we're okay with the non-Cartesian stuff looking a bit funny for a while, this is ready to go in. @brittonsmith , any thoughts? Next PR will switch to right-handedness for PWs.
If @brittonsmith approves I will merge.
Just so I'm clear, this is allowing us to fix righthandedness, but we are not actually fixing it in this PR?
Ok. Should we just go ahead and fix that at some point?