-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
def set_coord_type(self, coord_type): | ||
""" | ||
Set the coordinate type for the axis | ||
Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a full stop after axis
and an empty line before Parameters
@anizami - can you add a small image test? Take for example the MSX test image and change both axes to scalar, and set the format to e.g. I think you will need to also reset |
One more comment - can you add an optional |
@astrofrog - I've added what you suggested. Can you take a look at the image and see if it makes sense? I'm not sure what the behavior should be like if we change |
if coord_type == 'longitude' and coord_wrap is None: | ||
self.coord_wrap = 360 | ||
elif coord_type != 'longitude' and coord_wrap is not None: | ||
raise NotImplementedError('coord_wrap is not yet supported for non-longitude coordinates') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic has changed compared to the original code. Normally, coord_wrap should not be used for scalar coord_type either. Maybe you can try and keep the code more similar to before to keep the same logic?
@anizami - there are different use cases for this. For example, someone will have a WCS that doesn't say that the coord_type is longitude, which means it will default to scalar, and the user would then change to longitude. Or someone might want to change the coord_wrap from 360. to 180. (to make the longitude go from -180 to 180 instead of 0 to 360). Or maybe the user wants to treat the longitude as if it wasn't an angle. What you have here is good, except for my comment above about the fact the logic is changed. But once that's fixed, this should be good to go :) |
To answer your other question, |
@astrofrog - sorry about the changed logic, I've fixed that now :) |
@@ -246,3 +246,16 @@ def test_tick_angles_non_square_axes(self, generate): | |||
ax.coords['dec'].set_ticks(color='red', size=20) | |||
self.generate_or_test(generate, fig, 'tick_angles_non_square_axes.png') | |||
|
|||
# Test for setting coord_type | |||
def test_set_coord_type(self, generate): | |||
fig = plt.figure(figsize=(6, 6)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this smaller than 6,6 since it's not needed here? Maybe something like 3,3 would be enough (and you can make the font smaller to fit the labels if needed). Once you do this, make sure you rebase to squash the commits (to avoid having the larger image in the history).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
Looks good! Thanks @anizami! |
added method to set coord_type
added method to set coord_type
Does this look okay?