Skip to content
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

Fix example to follow latest analysis API #71

Merged
merged 3 commits into from
Aug 1, 2013

Conversation

astrofrog
Copy link
Contributor

Since the analysis properties are now quantities, we need to take their values. This makes me think that it might be nice to have it that one can transform a structure to a patch (ellipse) easily, e.g. PPStatistic.to_mpl_ellipse or something similar. @ChrisBeaumont - what do you think?

@astrofrog
Copy link
Contributor Author

Actually I'm curious why Matplotlib doesn't accept the quantities as they are. I also need to make sure that all the analysis code is compatible with Astropy 0.2.

@ChrisBeaumont
Copy link
Contributor

+1 on to_mpl_ellipse -- the code is ugly enough to hide behind a method

@astrofrog
Copy link
Contributor Author

Ok, will work on that!

@astrofrog
Copy link
Contributor Author

@ChrisBeaumont - I've now implemented to_mpl_ellipse - does this seem ok to you?

@ChrisBeaumont
Copy link
Contributor

This looks good.

Can you explain the benefit of the abc module? Is it just that it catches unimplemented methods earlier?

@astrofrog
Copy link
Contributor Author

@ChrisBeaumont - yes, the issue is that I didn't want to repeat the to_mpl_ellipse method, but it relies on things that only get implemented in sub-classes, it's a way to make sure (a) that you can't use the base class directly, and (b) that the methods get implemented in sub-classes.

@ChrisBeaumont
Copy link
Contributor

Thanks. This looks good to me

astrofrog added a commit that referenced this pull request Aug 1, 2013
Fix example to follow latest analysis API
@astrofrog astrofrog merged commit cebfd0d into dendrograms:master Aug 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants