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

New recipe: lens #2372

Merged
merged 14 commits into from
Mar 5, 2020
Merged

New recipe: lens #2372

merged 14 commits into from
Mar 5, 2020

Conversation

BeastyBlacksmith
Copy link
Member

@BeastyBlacksmith BeastyBlacksmith commented Jan 28, 2020

The lens recipe should magnify a certain region and show it in an inset plot.
Current API is

pl = plot((1:5).^2)
plot!(pl, [1.2, 2.8], [3.4, 7.8], seriestype=:lens, inset=(1,bbox(0.4,0.,0.5,0.5)))

Where you pass the magnifying region as x and y values and the location of the inset as usually.
Currently only relative bounding boxes are supported. Location of the linking line has to be improved.
Also drawing of the linking line should actually be outside of any plot, so it doesn't infer with axis limits of the parent plot, but I don't know if this is possible.
fixes #541

@@ -801,6 +801,50 @@ end
# note: don't add dependencies because this really isn't a drop-in replacement


# ---------------------------------------------------------------------------
# lens! - magnify a region of a plot
@recipe function f(::Type{Val{:lens}}, plt::AbstractPlot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this works - it's the type recipe signature, which is usually just used for type conversions to vector. Should this not be a "user recipe" with a @userplot shorthand defined?

Copy link
Member Author

@BeastyBlacksmith BeastyBlacksmith Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought, this is the signature of a plot recipe ( and a valid usecase )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I would like to only define the lens! shorthand, since lens by itself doesn't make too much sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right. I don't think they are used anywhere in the ecosystem, but I may be wrong (it lists MarginalHist as an example, but looking down on that page clearly shows that marginalhist is a "user recipe"). Defining a lens! shorthand makes sense - but I feel that calling it with seriestype as done here feels unideomatic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a more ideomatic alternative? I don't bother too much to change this.

Copy link
Member

@mkborregaard mkborregaard Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the lens! shorthand? Sorry I have zero experience using plot recipes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, now I got it. You meant the way to call the recipe is unideomatic, I thought the implementation with using the seriestype. I will add the lens! shorthand.

@BeastyBlacksmith
Copy link
Member Author

I thought, adding a ghost subplot for the connecting line would help, but, since the bounding box of the parent box seems to get calculated afterwards it still affects his axis limits. Also some backends have problems if the inset plot is outside of the parent plot.

@BeastyBlacksmith
Copy link
Member Author

Easiest course of action would be to get rid of the connecting line altogether and just have the box + inset.

@BeastyBlacksmith
Copy link
Member Author

This is the best I can do without having access to the bounding box of the parent plot.
Please give it a try.

@BeastyBlacksmith BeastyBlacksmith marked this pull request as ready for review January 31, 2020 22:27
@mkborregaard
Copy link
Member

@BeastyBlacksmith are you waiting for me here?
BTW would this close #541 ?

@BeastyBlacksmith
Copy link
Member Author

@mkborregaard Would be nice if you can confirm that I can't get the bounding box of the current plot at the recipe stage or if I am looking for the wrong thing and layout information is stored elsewhere.
Plus I am looking for a way to have the keywords passed be applied for only for the inset. I can make only parent or both but failed to do only inset.
Other than that #541 would be closed, yes.

@mkborregaard
Copy link
Member

Whether the bounding box is present depends on the recipe type. With this being a type recipe, I'm pretty sure it isn't - if it were a series recipe it would be present as a key in a captured plotattributes object (as they are processed later in the pipeline where the bounding box is known), but then series recipes won't let you make another window.
With regards to the keywords - try pop!ing them from the plotattributes Dict and store them locally, them passing them in the inset.

@BeastyBlacksmith
Copy link
Member Author

Ok, that worked. Could use some testing before merge.
An example is

pl = plot((1:5).^2)
lens!(pl, [1.2, 2.8], [3.4, 7.8], inset=(1,bbox(0.5,0.2,0.5,0.5)))

@mkborregaard
Copy link
Member

You could add a testing image to examples maybe? That way it would become part of the testing suite - and be documented...

@BeastyBlacksmith
Copy link
Member Author

I added the example and made sure, I can easily revert the formatting changes, if they don't belong here.

src/examples.jl Outdated
@@ -311,6 +311,21 @@ const _examples = PlotExample[
],
),
PlotExample(
"Lens",
"A lens lets you easyli magnify a region of a plot. x and y coordinates refer to the to be magnified region and the via the `inset` keyword the subplot index and the bounding box (in relative coordinates) of the inset plot with the magnified plot can be specified. Additional attributes count for the inset plot.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

src/examples.jl Outdated
end
],
),
* PlotExample(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray *

@mkborregaard
Copy link
Member

Nice - would you mind splitting the formatting changes to a separate PR?
Are you familiar with how to add a figure to PlotReferenceImages to make tests pass? (examples double as tests)

@BeastyBlacksmith
Copy link
Member Author

BeastyBlacksmith commented Mar 4, 2020

Are you familiar with how to add a figure to PlotReferenceImages to make tests pass? (examples double as tests)

No, never did this.

@daschw
Copy link
Member

daschw commented Mar 4, 2020

Firstly, I'd suggest putting new test_examples at the end of Plots._examples, because the test examples in PlotReferenceImages are named after their index in that Array. This way the proper images are compared to each other.

To update the PlotReferenceImages, you basically have to

  1. ]dev PlotReferenceImages, fork it, create a new branch, ...
  2. Run ]test Plots
  3. If images differ a GUI opens show ing the old and new image and asking you if you want to save the changes.
  4. After tests finished, add new images in your PlotReferenceImages branch, push to fork and PR

After the PlotReferenceImages PR is merged, the tests of this PR will pass

@BeastyBlacksmith BeastyBlacksmith merged commit 55ea8b4 into master Mar 5, 2020
@BeastyBlacksmith BeastyBlacksmith deleted the bbs/lens-recipe branch March 6, 2020 12:01
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.

subplot views
3 participants