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.

Sign up for GitHub

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

Jump to bottom

jointplot now respects the ratio argument (top plot is larger) with matplotlib backend #682

Merged
merged 6 commits into from
Oct 23, 2019
Merged

jointplot now respects the ratio argument (top plot is larger) with matplotlib backend #682

merged 6 commits into from
Oct 23, 2019

Conversation

DougBurke
Copy link
Contributor

@DougBurke DougBurke commented Sep 23, 2019 •
edited
Loading

Summary

The matplotlib back end now makes the main (top) plot taller than the secondary (bottom) plot when using the sherpa.ui.plot_fit_resid and sherpa.ui.plot_fit_delchi routines (this also holds for the sherpa.astro.ui variants).

Details

With this change the sherpa[.astro].ui.plot_fit_resid/delchi routines now display with the top plot being twice the height of the bottom plot when using the matplotlib (pylab) backend. The code was just not applying the requested ratio argument.

There is no test for this as there is currently no good test suite for the plot contents when a plotting backend is available.

This is to address #587 and collides with #626 (although easy to address).

An example of what sherpa.astro.ui.plot_fit_delchi looks like with this PR (examples from before the change are available in #587):

resized

The commits should be squashed into a single commit before merging, but left separated for initial review. There is no documentation on what a "joint plot" is meant to represent, so unclear how to handle the nrows and ncols arguments to set_jointplot.

@DougBurke
Copy link
Contributor Author

I cancelled the tests for the 4614e4a build because I realized I needed to add a change, and then one of the tests failed due to network timeouts, so it was easier to just cancel the whole build and let the 697430a tests run instead.

@wmclaugh wmclaugh added this to the 4.12.0 milestone Sep 26, 2019
@DougBurke DougBurke mentioned this pull request Sep 27, 2019
Merged
@DougBurke
Copy link
Contributor Author

I'm going to rebase this to fix the conflict.

DougBurke and others added 5 commits October 9, 2019 15:02
@DougBurke
mpl jointplot now respects the ratio argument (top plot is larger)
6c6b936 
With this change the sherpa[.astro].ui.plot_fit_resid/delchi routines
now display with the top plot being twice the height of the bottom
plot when using the matplotlib (pylab) backend.

There is no test for this as there is currently no good test suite
for the plot contents when a plotting backend is available.
@DougBurke
Handle jointplots with more than 2 elements
a36d290 
This follows the chips backend for determining the relative height.
@DougBurke
Matplotlib and chips backends disagree on the number of subplots
7789e40 
It is not obvious whether jointplot is meant to be used with anything
but a single column: the chips backend seems to suggest it does, but
then only creates a single column anyway, and the matplotlib backend
just ignores the number-of-columns argument (at least in set_jointplot).

Follow the matplotlib code here.
@DougBurke
Docs: add an example of JointPlot usage
ae95c81
@DougBurke
Docs: re-run notebook example to get fixed plot
b0894a3 
The Sherpa notebook uses the plot_fit_resid command so was re-run
to pick up the updated plot. A link to the read-the-docs Sherpa
documentation was added.
@anetasie anetasie self-assigned this Oct 10, 2019
@anetasie anetasie self-requested a review October 10, 2019 00:22
@anetasie anetasie removed their assignment Oct 10, 2019
wmclaugh
wmclaugh reviewed Oct 10, 2019
View reviewed changes
docs/fit/index.rst Outdated Show resolved Hide resolved
@DougBurke
Docs: fix pcall typo
cc67c14 
Change `pcall` to `call` as noted by @wmclaugh
@wmclaugh wmclaugh merged commit eb4ae4d into sherpa:master Oct 23, 2019
@DougBurke DougBurke deleted the resize-split-plots branch October 23, 2019 12:33
@DougBurke DougBurke mentioned this pull request Oct 23, 2019
Closed
Marie-Terrell pushed a commit that referenced this pull request Nov 12, 2019
@wmclaugh @Marie-Terrell
Merge #682 (DougBurke) - jointplot now respects the ratio argument (t…
9922658 
…op plot is larger) with matplotlib backend

jointplot now respects the ratio argument (top plot is larger) with matplotlib backend
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Sep 4, 2020
@DougBurke
Support overplot option in plot_fit_xxx (issue sherpa#700)
a3b2ba2 
This allows you to say

    plot_fit_resid(ylog=True)
    plot_fit_resid(2, overplot=True)

The sherpa.plot.JointPlot class has been reworked to explicitly
support the overplot option. There is no existing documentation
so it's not entirely obvious how the JointPlot and parent
SplitPlot class are meant to work, but the tests still pass
(and I've added a few to explicitly check the joint-plot case).

Note that I have taken the liberty to change the signature of the
set_jointplot routine (that is now only defined for matplotlib now
that chips has gone away, and wasn't documented). The optional
top parameter, which was never set, has been changed to match the
numbering scheme of row and column. This routine was fixed in
PR sherpa#682 for the Matplotlib backend but at that time it only supported
a single column (both before and after sherpa#682 ws fixed), but this has
now been fixed (although we have no support for this used with any
other column count).
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Sep 4, 2020
@DougBurke
Support overplot option in plot_fit_xxx (issue sherpa#700)
59ffa66 
This allows you to say

    plot_fit_resid(ylog=True)
    plot_fit_resid(2, overplot=True)

The sherpa.plot.JointPlot class has been reworked to explicitly
support the overplot option. There is no existing documentation
so it's not entirely obvious how the JointPlot and parent
SplitPlot class are meant to work, but the tests still pass
(and I've added a few to explicitly check the joint-plot case).

Note that I have taken the liberty to change the signature of the
set_jointplot routine (that is now only defined for matplotlib now
that chips has gone away, and wasn't documented). The optional
top parameter, which was never set, has been changed to match the
numbering scheme of row and column. This routine was fixed in
PR sherpa#682 for the Matplotlib backend but at that time it only supported
a single column (both before and after sherpa#682 ws fixed), but this has
now been fixed (although we have no support for this used with any
other column count).
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Oct 3, 2020
@DougBurke
Support overplot option in plot_fit_xxx (issue sherpa#700)
8052d2f 
This allows you to say

    plot_fit_resid(ylog=True)
    plot_fit_resid(2, overplot=True)

The sherpa.plot.JointPlot class has been reworked to explicitly
support the overplot option. There is no existing documentation
so it's not entirely obvious how the JointPlot and parent
SplitPlot class are meant to work, but the tests still pass
(and I've added a few to explicitly check the joint-plot case).

Note that I have taken the liberty to change the signature of the
set_jointplot routine (that is now only defined for matplotlib now
that chips has gone away, and wasn't documented). The optional
top parameter, which was never set, has been changed to match the
numbering scheme of row and column. This routine was fixed in
PR sherpa#682 for the Matplotlib backend but at that time it only supported
a single column (both before and after sherpa#682 ws fixed), but this has
now been fixed (although we have no support for this used with any
other column count).
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Oct 4, 2020
@DougBurke
Support overplot option in plot_fit_xxx (issue sherpa#700)
87ebb57 
This allows you to say

    plot_fit_resid(ylog=True)
    plot_fit_resid(2, overplot=True)

The sherpa.plot.JointPlot class has been reworked to explicitly
support the overplot option. There is no existing documentation
so it's not entirely obvious how the JointPlot and parent
SplitPlot class are meant to work, but the tests still pass
(and I've added a few to explicitly check the joint-plot case).

Note that I have taken the liberty to change the signature of the
set_jointplot routine (that is now only defined for matplotlib now
that chips has gone away, and wasn't documented). The optional
top parameter, which was never set, has been changed to match the
numbering scheme of row and column. This routine was fixed in
PR sherpa#682 for the Matplotlib backend but at that time it only supported
a single column (both before and after sherpa#682 ws fixed), but this has
now been fixed (although we have no support for this used with any
other column count).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wmclaugh wmclaugh wmclaugh left review comments

@anetasie anetasie anetasie approved these changes

@dtnguyen2 dtnguyen2 dtnguyen2 approved these changes

Assignees
No one assigned
Labels
area:code general:visualization
Projects
None yet
Milestone
4.12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@DougBurke @anetasie @dtnguyen2 @wmclaugh