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: https://github.com/racket/plot/issues/83#issuecomment-781037659

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?

[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 (https://github.com/racket/plot/issues/83#issuecomment-790017560). 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.

[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.