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

Add sample plots to built-in models #4008

Closed
wants to merge 11 commits into from
Closed

Add sample plots to built-in models #4008

wants to merge 11 commits into from

Conversation

@sYnfo
Copy link
Contributor

sYnfo commented Jul 25, 2015

I will add more plots as I finish them. Feel free to point out any improvements that could be made and if you would like to help with adding more sample plots, please open a PR against sYnfo/astropy:issue3894. :)

Resolves #3894.

@sYnfo

This comment has been minimized.

Copy link
Contributor Author

sYnfo commented Jul 25, 2015

Sine1D sample plot

Sine1D sample plot

@embray

This comment has been minimized.

Copy link
Member

embray commented Jul 27, 2015

Something I nearly forgot about with regard to these, is in #3200 we added a "standard" matplotlib style for Astropy. We should make sure that examples in the astropy docs are using this style too (with exceptions of course if it looks like crap for any particular plot).

@sYnfo

This comment has been minimized.

Copy link
Contributor Author

sYnfo commented Aug 1, 2015

I've started to use the astropy mpl style, although I'm not sure if I'm using it correctly, they seem to look the same.

Gaussian1D

Gaussian1D

I'm not super happy with how this particular sample plot merges with the rest of the Examples section. I wonder if it would be a good idea to add a new "Sample plot" section, separate from general example of usage.

sYnfo added 2 commits Aug 1, 2015
@sYnfo

This comment has been minimized.

Copy link
Contributor Author

sYnfo commented Aug 1, 2015

I've reverted the astropy mpl style commit for now, as it was failing tests. The current failure isn't my fault, I think.

GaussianAbsorption1D

GaussianAbsorption1D

@mdboom

This comment has been minimized.

Copy link
Contributor

mdboom commented Aug 3, 2015

Having seen this, I'm working on a separate PR to enable the astropy style everywhere. Stay tuned... I'd just continue with what you are doing for now.

Part of the reason your plots are looking the same with/without the style is that they are explicitly setting the line colors (which they should be -- the shades of grey is the best way to illustrate your point).

@eteq

This comment has been minimized.

Copy link
Member

eteq commented Aug 3, 2015

@mdboom - you said you're already working on this, but just in case: do you know that we are applying uniform styles on astropy-tutorials , and some of that machinery might be reusable here? (Regardless, 👍 to the idea)

@mdboom

This comment has been minimized.

Copy link
Contributor

mdboom commented Aug 3, 2015

@eteq: The machinery amounts to three lines of configuration in conf.py, since the style itself lives in astropy.visualization. The semi-time-consuming bit is updating all of the existing plots to use the style and make sure everything is still legible etc...

@embray

This comment has been minimized.

Copy link
Member

embray commented Aug 3, 2015

Maybe shades of orange instead of shades of gray? ;) (Although honestly whatever is most actually legible is best...)

@pllim

This comment has been minimized.

Copy link
Member

pllim commented Aug 17, 2015

Maybe shades of orange instead of shades of gray?

50 shades of gray...

@embray embray force-pushed the astropy:master branch from 63540e0 to 34f4c29 Oct 5, 2015
@hamogu

This comment has been minimized.

Copy link
Member

hamogu commented Jan 29, 2016

@embray Can this be merged? Yes, it might not be perfect or complete, but it
is a big step forward. Perfect is the enemy of good.
There has been no activity on this for ~6 months, so I suggest merging as is and tweaking / adding more plots in a later PR.

@astrofrog

This comment has been minimized.

Copy link
Member

astrofrog commented Apr 18, 2016

I agree with @hamogu - I'm going to add a changelog entry and merge this manually

astrofrog added a commit that referenced this pull request Apr 18, 2016
Add sample plots to built-in models
@astrofrog

This comment has been minimized.

Copy link
Member

astrofrog commented Apr 18, 2016

Merged manually in cc7977e

@astrofrog astrofrog closed this Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.