Plotmetrics

[bdeket]

Possible implementation for getting plot-metrics from plots. see #83

Currently implemented methods:

• get-plot-boundaries: (-> (Vectorof (Vector Real Real)) - give min/max x y (z) values
• plot->dc: (-> (Vecorof Real) (Vectorof Real)) - translate plot x y (z) coordinates to dc x y coordinates

Possible problems:

• This changes the return type for plot/dc, plot-bitmap, plot-snip, plot-pict
• typed-untyped interaction: Except for plot-pict the return types are new objects for which no interface or class is defined. There will be no straighforward way to check if the returned objects implement Plot-Metrics<%>

todo:

• documentation
• tests

[alex-hhh]

Thanks for starting the work on this feature. Unfortunately, I will only have time to look at this in detail next week, but it would be good to get the rest of the Racket teams opinion on this approach. My own take is that the overall approach is OK, and it does solve some of the challenges with returning extra information with the plot itself.

[samth]

A couple thoughts.

1. It seems like it would be easy to add an interface with the two relevant methods to all the new classes and make it easy to test for in untyped code that way.
2. I think an abstraction for creating the classes might be nice, something like

(define (plot-metrics-mixin c bounds ->dc)
    (class c (plot-metrics<%>) 
        (super-new)
        (define/public (get-plot-bounds) (bounds)
        (define/public (plot->dc [v : (Vectorof Real)]) (->dc v))))

[bdeket]

@samth Currently plot/dc and plot-bitmap are defined in TR, and plot-snip is defined in untyped racket. That makes it hard to do what you are suggesting, right? (This was also the reason of my question about interfaces and TR yesterday)

[alex-hhh]

Hi @samth, is it OK to change the return type of plot/dc from void to a plot-metrics<%> object? I think it is OK, but @rfindler indicated in ths comment that it might not be a good idea, but did not explain why: #83 (comment)

Another question for @samth: this is a complex feature, and it is not clear to me that these changes will be sufficient or even useful for the types of things people have requested them. I don't think we can get this right in one go, since we have only one use case:

• ideally, I would like @dented42 to try these changes out, but I am not sure if he/she still has time and he/she is interested in this
• finish and merge this implementation to the main branch and add it to a Racket release, so other people can experiment with it
• mark this feature as experimental in the Racket release, and allow changing these APIs in non-backwards compatible ways based on feedback from users who try to use these features

What do you think?

[alex-hhh]

I think the structure should be named plot-pict as this is more consistent with other names.

[alex-hhh]

oops, plot-pict is a function name...

[alex-hhh]

We also need a dc->plot method, since that would be used to convert mouse coordinates to the plot space.

[bdeket]

What would be the return type be for plot3D, a line?

[bdeket]

Or alternatively: apart from adding dc->plot, also provide a plane-vector method. For 2D this would always return #(0 0 1), for 3D this would return the unit vector e that is perpendicular to the screen. So for dc->plot 3D can just return the coordinate c0 for the plane trough the origin. Then the user can reconstruct the right coordinate by adding these two together: (+ c0 (* a e))

[alex-hhh]

I did not think about the 3D case and the 2D plot area already has a dc->plot method.

I am thinking about this feature in terms of "what functionality is needed to construct interactive plots?" since this was the original use case for creating #83. I have some experience with this, from using the plot-snip% interface for 2D plots, and I know that there needs some way from converting from DC to plot coordinates, so be able to interact with the mouse.

For 3D plots, I have no experience with them, and not sure what is needed to achieve the final goal. Making dc->plot return a line is the mathematically correct thing to do, but I am not sure if this would be sufficient and/or useful for anything practical.

Perhaps we could not do the 3D plot changes at all, and wait until someone comes with a use case that we can use to validate the design?

[alex-hhh]

I think the bounds should be calculated and memory allocated for them in the get-plot-bounds method below, perhaps lazily initialize them as a member of the returned plot-metrics object.

The plot/dc method is used in every 2D plot and most of these uses will just discard the plot-metrics object so I want to avoid the memory allocation penalty (and runtime execution penalty) in all those use cases.

[alex-hhh]

While creating a new class type and immediately instantiating an object is supported by Racket, I am not sure if this is the most efficient way to do it in terms of memory allocation and runtime performance -- this object will be discarded in most current uses of the plot package).

@samth do you know whether it would be more efficient to create a separate plot-metrics% class and just instantiate this here?

[samth]

@mflatt will know better here, but it will at most save a bit of allocation here.

[alex-hhh]

We only have one use case for these changes (#83 (comment)) and this would involve calling plot/dc several times a second (30FPS perhaps). I think this code should avoid any unnecessary memory allocations and defer any deferrable computations until they are needed (e.g. calculating the bounds).

[alex-hhh]

same notes about allocating bounds as for the 2D plot.

[alex-hhh]

... and for creating a new class and instantiating it...

[samth]

@bdeket I don't really understand your comment. You can define the interface in untyped code, and then ensure that it's implemented by all the classes. There would be some duplication, which is unavoidable here for the reasons I described on Slack, but that doesn't seem so bad here.

[samth]

@alex-hhh On the backwards-compatibility issue, I think it's unlikely to be a problem. I agree with @rfindler (where did his comment go?) that the most likely problem is someone with a contract that starts failing, but I still think that's unlikely.

On the experimental issue, I'm less sure. We should think about what changes we might want to make that wouldn't be possible in a compatible way (say, by adding another method).

[bdeket]

@samth I didn't understand how to do what you were proposing. But I think I do now:
Create interface and a mixin (or otherwise class) in untyped racket.
require-typed them in the TR modules and apply them there.

My initial thought was only having the interface in untyped code, and I didn't see how to integrate that with the other pieces.

[alex-hhh]

Hi @samth

On the experimental issue, I'm less sure. We should think about what changes we might want to make that wouldn't be possible in a compatible way (say, by adding another method).

My concern is that this is a large and complex change, and we only have only one use case for it (#83 (comment)). There is a distinct possibility that, apart from perhaps a few tests and toy examples, no one will use this, and, when they will, we'll discover that, despite our best efforts, the interface is not sufficient or not useful.

As such, I would like to get these changes out in a Racket release (since most people won't build the plot package themselves), announce them, allow people to try them out and provide feedback then incorporate that feedback in a future plot package release.

[alex-hhh]

Hi @bdeket , I think the changes look good. Can you please add some unit tests for these, I think we should leave the documentation update for a separate pull request, as this one is getting too large.

[bdeket]

Hi @alex-hhh, I will have time tomorrow/later this week.
But I was wondering, regarding trying out this changes in a release. Would it make sense to setup a plot/unstable where this change could be tested for some time, before being merged into the main branch. The bad part is, that it will involve some code duplication

[alex-hhh]

I also thought about creating a plot/unstable module for these changes, but it might not be worth the effort, since it requires a lot of changes.

Another option is to not merge this branch yet, and, when someone asks for this feature in the future, point them to this branch and complete the work at that time based on their feedback. The plot package does not change that often and it would be simple to keep this branch up to date with the main branch. I am not sure how many people would use this feature in the short to medium term (say 6 - 12 months), the person who originally created the issue has not responded yet, and it may be that they are no longer interested.

Finally, I am also happy to just merge this. When someone starts using these features, we'll get some feedback and decide at that time what to do if the API needs to be changed.

[mfelleisen]

Don't create an unstable module/directory. We had one at the top level and it leads to the usual problems.

[alex-hhh]

Unfortunately, this sleep call will create a flaky test which might fail depending on what machine it is run on and the overall CPU load on that machine. Unfortunately, I don't have a suggestion on how to carry out this test, so I think this should be disabled for now.

I think the testing is sufficient for now.

[bdeket]

Will it be ok if I leave the test in as a comment?

[alex-hhh]

Yes, it should be left in as a comment -- it would be a good idea to be able to test these things, but I don't know how.

Also, you will need to merge the master branch into this one.

[samth]

There ought to be something that can wait for the resize to complete -- maybe @rfindler knows.

[samth]

Returning to the "marking things unstable" issue, after thinking about this more and talking with the rest of the core team, I think that just noting in the documentation that this API is unstable in the 8.1 release is very reasonable. Of course, just committing to it now is also fine, and I'll leave that judgment up to you.

[alex-hhh]

I will consider any backwards incompatible changes very carefully, and avoid them if at all possible, but, given that this change will not be validated with real use-cases, there is a small possibility that we got this interface wrong.

[dented42]

Sorry for the delay, alas it looks like I missed all the action. Regardless @alex-hhh this handles everything that I was thinking of.

My concern is that this is a large and complex change, and we only have only one use case for it (#83 (comment)). There is a distinct possibility that, apart from perhaps a few tests and toy examples, no one will use this, and, when they will, we'll discover that, despite our best efforts, the interface is not sufficient or not useful.

I'm no expert on API design, but this implementation seems to me to have enough wiggle room that it can be modified or extended with a new approach that feels closer to the platonic ideal of this sort of a thing.