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

ComputedSet (values-only ComputedPairs) #10

Closed
dphfox opened this issue Aug 8, 2021 · 7 comments · Fixed by #107
Closed

ComputedSet (values-only ComputedPairs) #10

dphfox opened this issue Aug 8, 2021 · 7 comments · Fixed by #107
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made

Comments

@dphfox
Copy link
Owner

dphfox commented Aug 8, 2021

Right now, developers using ComputedPairs have to be careful not to use unstable keys. This can subtly cause extra recalculations:

local data = State({"Red", "Green", "Blue", "Yellow"})

print("Creating processedData...")

local processedData = ComputedPairs(function(key, value)
    print("  ...recalculating key: " .. key .. " value: " .. value)
    return value
end)

print("Removing Blue...")
data:set({"Red", "Green", "Yellow"})

UnstableKeys-bg

Right now, the solution to this problem is to make the keys stable, usually by using the values as keys:

local data = State({Red = true, Green = true, Blue = true, Yellow = true})

print("Creating processedData...")

local processedData = ComputedPairs(function(key)
    print("  ...recalculating key: " .. key)
    return key
end)

print("Removing Blue...")
data:set({Red = true, Green = true, Yellow = true})

StableKeys-bg

(see https://elttob.github.io/Fusion/tutorials/further-basics/arrays-and-lists/#optimisation)

An alternate solution could be to introduce a ComputedSet object (name open to bikeshedding) - this would act like ComputedPairs, but would ignore keys completely and cache values instead. The values would still have to be non-nil and unique however.

Hypothetical usage:

local data = State({"Red", "Green", "Blue", "Yellow"})

print("Creating processedData...")

local processedData = ComputedSet(function(value)
    print("  ...recalculating value: " .. value)
    return value
end)

print("Removing Blue...")
data:set({"Red", "Green", "Yellow"})
@dphfox dphfox added enhancement New feature or request not ready - evaluating Currently gauging feedback labels Aug 8, 2021
@boatbomber
Copy link
Contributor

boatbomber commented Aug 8, 2021

Potentially pretty valuable (especially if we can get ipairs internally for some extra speed), but the workaround of [val] = true seems good enough for now. This definitely isn't a blocking issue, but could be implemented in the second release!

@dphfox
Copy link
Owner Author

dphfox commented Dec 19, 2021

I've decided on an API design for this after much consideration with users.

I plan to replace ComputedPairs with 3 new mapping objects (names being bikeshedded).


ForKeys

Iterates over every pair of the input table. Transforms the key using the processor function (KI) -> KO. Puts transformed key and original value in output table.

Processing only occurs when a key is added. Transformed keys are cached. Values do not trigger processing but are forwarded to the output table directly.

Ideal for transforming sets, where elements are stored as keys.

ForValues

Iterates over every pair of the input table. Transforms the value using the processor function (VI) -> VO. Puts original key and transformed value in output table.

Processing only occurs when a key is added or when a key's value changes to a non-nil value. Transformed values are cached, though care needs to be taken with handling duplicate input values to ensure every value in the output table is unique.

Ideal for transforming unordered arrays, where order does not matter but duplicates are allowed.

ForPairs

Iterates over every pair of the input table. Transforms the pair using the processor function (KI, VI) -> (KO, VO). Puts transformed pair in output table.

Processing only occurs when a key is added or when a key's value changes to a non-nil value. Transformed pairs are cached by key.

Ideal for transforming ordered arrays and dictionaries, where both keys and values carry meaning.


Under this new system there is no direct replacement for ComputedPairs. While this may sound bad, it's actually great - it forces users to consider which behaviour they need, which means Fusion can better optimise for each case without user intervention or cognitive load.

@dphfox dphfox added ready to work on Enhancements/changes ready to be made and removed not ready - evaluating Currently gauging feedback labels Dec 19, 2021
@Dionysusnu
Copy link
Contributor

Ideal for transforming unordered arrays, where order does not matter but duplicates are allowed.

In ForValues, you say this. But doesn't this introduce the initial issue of subtle recalculations?

@dphfox
Copy link
Owner Author

dphfox commented Dec 19, 2021

Ideal for transforming unordered arrays, where order does not matter but duplicates are allowed.

In ForValues, you say this. But doesn't this introduce the initial issue of subtle recalculations?

Nope! The initial issue of subtle recalculations is down to keys being assumed to carry meaning, which they don't in ForValues.

To illustrate:

local array = Value({"Red", "Green", "Blue"})

ComputedPairs(array, function(key, value)
    -- notice ComputedPairs gives you the key
    -- ComputedPairs has to assume the key could be used to calculate the returned value here
    -- therefore, we can't reuse a value if it appears at a different key
    return {
        colour = value
    }
end))

ForValues(array, function(value)
    -- here, ForValues doesn't give us the key
    -- the key could never be used to calculate the returned value here
    -- therefore, we *can* reuse a value if it appears at a different key
    return {
        colour = value
    }
end))

Specifically with respect to duplicate values, the only condition is that two values in the output array never point to the same cached object. ForValues doesn't have to make any further guarantees.

@nottirb
Copy link
Contributor

nottirb commented Dec 19, 2021

Do we want the optional destructor to be called with the output of For*? Since that's how ComputedPairs currently works:

processor: (K, VI) -> VO,
destructor: (VO) -> ()?

So the idea would be as follows,

For ForKeys:

processor: (KI) -> KO,
destructor: (KO) -> ()?

For ForValues:

processor: (VI) -> VO,
destructor: (VO) -> ()?

For ForPairs:

processor: (KI, VI) -> (KO, VO)
destructor: (KO, VO) -> ()?

@Dionysusnu
Copy link
Contributor

Dionysusnu commented Dec 19, 2021

If we're on it, this is a good time to implement #97 along with it. But since that's not quite approved yet, maybe this issue should wait a bit.

@dphfox
Copy link
Owner Author

dphfox commented Dec 20, 2021

Yes - I think that the destructor should be forwarded all returned values from the processor function, including any extra metadata the user wishes to provide (see #97).

@dphfox dphfox linked a pull request Feb 3, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants