magland / figurl-timeseries-views

0 stars 0 forks source link

Implement separate x- and y-converters, updated less often. #6

Open jsoules opened 1 year ago

jsoules commented 1 year ago

Doesn't need to be merged at this point--I just wanted to report back.

I made some changes on a separate branch that created the kind of interface described in Issue #5. Unfortunately, the matrix version proved to be somewhat slower. I'm not sure why that is, especially as I've observed the opposite on other occasions; I would like to profile it further, but I don't think it's worth the effort right now, however much it bothers me.

This PR introduces a couple minor changes to separate computation of the x- and y-dimension pixel values into separate functions, and redefine those callbacks less often. Rationally, this would be expected to result in some speedup--if nothing else, the existing version has to compute both dimensions (and then throw one away) for each data series--but the times are pretty much indistinguishable, so I'm not sure it actually makes sense to make the change.

I'm at a loss for this...

magland commented 1 year ago

This code looks good to me. Let me know when is the right time to merge.

jsoules commented 1 year ago

It's less that it needs to be fixed, as that it doesn't really seem to help anything... I'm not sure if that's a good basis for merging?

magland commented 1 year ago

Okay, I guess if it doesn't help then we shouldn't add additional functions to the code. What I was thinking would be the cleanest way to do it is:

const coordToPixel = (() => {
    // coefficients a, b, c, d are defined here
    return (p: {x: number, y: number}): {x: number, y: number} => {
        return {
            x: a + b * p.x,
            y: c + d * p.y
        }
    }
})()

Then everything is all in one place, and it's as efficient as it can be.

jsoules commented 1 year ago

That's pretty much what I did, except doing the x- and y-dimensions separately so that we don't have to compute against a dummy 0 value. I'm just surprised that it doesn't seem to affect the runtime.

magland commented 1 year ago

Ah okay I see. Well, I guess we can leave the code as it is, except we'll keep this as a record of what was tried.