New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvement in Plotting methods. #194
Conversation
632588c
to
d2c49c6
Compare
d796309
to
a5758e4
Compare
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.
This is looking really good. I'm not sure I caught everything in this review; there's a lot changed here.
Could you add an example to the docs with these improved plots? Something like the images on the frontpage of the pyregion docs. That might be best as the topic of another PR, but we should show off these changes somewhere!
patch = mpatches.PathPatch(path, **kwargs) | ||
return patch | ||
else: | ||
raise NotImplementedError |
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.
Is this supposed to be reachable? If not, see if you can add a message that might help the user figure out what went wrong. I don't believe non-concentric annuli should be possible to declare.
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.
yes, it's reachable. It is the CompoundPixelRegion
class and not CircleAnnulusPixelRegion
regions/io/crtf/read.py
Outdated
else: | ||
self._raise_error("Not in proper format: '{0}' should be a symbol".format(y)) | ||
elif self.region_type == 'text': | ||
self.meta['text'] = y |
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 we maybe use more descriptive variable names than x
and y
here? That might be the subject of a different PR, I just realize now looking at this code that it's hard to understand where those come from with these names.
from matplotlib.patches import Arrow | ||
x = self.start.x | ||
y = self.start.y | ||
dx = self.end.x - self.start.x | ||
dy = self.end.y - self.start.y | ||
if not 'width' in kwargs: | ||
kwargs['width'] = .1 # Let the default width be .1 instead of 1. |
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.
Why? I don't mind, just wondering if there's a reason. Does that make it look more like ds9?
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.
It does not look like a line
with width one. This Arrow
patch seems ugly.Also, there is a line
meta in DS9 decides which end of the line would be arrow like. I will work on that.
regions/io/core.py
Outdated
return _to_io_meta(region_meta, region_visual, valid_keys, key_mappings) | ||
if 'font' in meta: | ||
meta['font'] += " {0} {1} {2}".format(shape_meta.get('fontsize', 12), | ||
shape_meta.get('fontstyle', 'normal'), |
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.
whitespace (indentation isn't consistent here)
a5e4460
to
6c512ae
Compare
There are a lot of big and small changes made here, and I don't want to push things further in the PR here. |
I'm happy with this, so I'm merging it. Could you link to the Issues with the next steps? For one, we want a documentation page analogous to pyregion's frontpage showing all the graphical tools in action; it can pretty much be the RST version of the original issue posted here. You also left a to-do item of having some of the internals of the plot method put into an |
added
as_patch
for annulus.Now, also uses visual meta data for plotting:
Also, implemented the
__eq__
forPixCoord
.