Allow infinite count of items #656

xsebek · 2022-08-31T23:30:28Z

use integers extended with negative and positive infinity for inventory count and numbers in-game
fix hashing
add infinity constant
- make it the same as in JSON
- pretty print infinities using this constant
add integration test with infinities in scenario
closes Infinite items in inventory #621

byorgey · 2022-09-01T21:24:56Z

I don't understand why Natural makes it crash, can you elaborate?

src/Swarm/Game/Entity.hs

src/Swarm/Game/Step.hs

byorgey · 2022-09-04T21:24:41Z

Random thought: what if the Swarm int type included positive and negative infinity? That would solve the problem with count having to return an arbitrary value when you have infinity of something. We'd have to consider carefully how arithmetic would work, and we'd probably have to add some new primitives like isInf. But it doesn't seem like an immediately terrible idea.

byorgey · 2022-09-04T21:46:43Z

Let me see if I can write down a coherent explanation of what's going on with the hashing. The basic issue is with hashing inventories. We want our hashes to satisfy two main criteria:

Two unequal inventories should have a high probability of having different hashes.
It should be possible to update the hash of an inventory in O(1) when an entity is added or removed.

Note that inventories are considered unequal if there is at least one entity that the two inventories have different numbers of. However, in this case the "number of" an entity can include any nonnegative value but ALSO a special "Nothing" value meaning "I have never heard of this entity". We are now proposing to add yet another special value, positive infinity.

Criterion (2) above is what makes things interesting. Without that criterion everything becomes easy, in fact, we could auto-generate the hashing code for an inventory. But the auto-generated code iterates through the whole inventory and combines hashes for entities in a linearly chained way, so inserting or removing an entity means iterating through the entire inventory to recompute the hash from scratch. #294 fixed this by switching to a homomorphic hashing scheme for inventories. Instead of combining hashes by concatenating them and running the hash function again (say), we simply add them. This is less cryptographically secure but we don't care about that, and it gives us criterion (2): given the existing hash for an inventory, if we insert an entity, just add the entity's hash to the inventory hash. Also, if we add 100 copies of a given entity, we can just multiply the hash by 100 and then add that, rather than adding it 100 times.

Currently, if we have k copies of an entity in the inventory, it contributes (k+1) times the entity's hash to the inventory hash. The reason for the k+1 is to accommodate the difference between not knowing about an entity at all (in which case its hash is not added to the inventory hash at all) and knowing about it but having 0 (in which case its hash is added once). This works though since it is homomorphic with respect to adding and removing entities. If we insert one copy of an entity into an inventory, we just add its hash to the inventory hash. Say we used to have 3 copies, which means 4 times its hash was contributed to the inventory hash... so now 5 times its hash will be contributed, which corresponds to having 4 copies, just like it should.

However, positive infinity throws a wrench into things. I think we can make it work but we need to check more cases. As a concrete proposal, let h denote the hash of an entity, and suppose we use

0 to represent not knowing about an entity
1h to represent having infinity of the entity
2h to represent having 0
3h to represent having 1
... (k+2)h to represent having k

So far, so good. But one problem is that this is not homomorphic with respect to adding and removing. Suppose we have infinity copies of an entity, and we insert one more. Now we should still have infinity, but that means we can't just add a copy of the hash every time we insert. We have to first check to see how many copies we have before the insert.

If we didn't know about the entity before, we should add 3h to the inventory hash.
If we had some finite number of copies, we add h to the inventory hash.
If we had infinite copies, we don't change the inventory hash.

Taking a union of inventories is also more complex. Honestly it seems like when unioning two inventories we might as well go ahead and recompute the hash of the result from scratch, since it is not worth the trouble of trying to update incrementally.

xsebek · 2022-09-05T07:25:29Z

what if the Swarm int type included positive and negative infinity?

I am starting to like that idea. It would make it easier to use code written with the expectation that Count = Int and make division safer in the common case 1/0. There would be one funny case of 1/0 - 1/0, which is not that likely to bother anyone.

xsebek · 2022-09-05T07:39:34Z

... If we had infinite copies, we don't change the inventory hash.

Well, that is annoying. Maybe we can use updateLookupWithKey to get the new value and if it is the same infinity we do not add the hash?

byorgey · 2022-09-06T03:10:55Z

Well, that is annoying. Maybe we can use updateLookupWithKey to get the new value and if it is the same infinity we do not add the hash?

Yeah, I think something like that could work.

xsebek · 2022-10-18T16:57:34Z

Rebased and changed numbers to use negative and positive infinity. 🙂

Btw. this works now in creative mode:

l <- count "Linux"; return $ l == 1/0

This approach is buggy in two ways: - it can crash because Natural is too strict - it breaks the fragile hashing laws for inventory A better approach would be to either do without a Num instance and handle every usage carefully or add negative infinity too and use integers. In that case we might as well add them to the number values in game, though that increases the risk of the infinities being added together. But that can be handled with a safeAdd function.

xsebek · 2022-10-18T17:26:33Z

Rebased again to see if the VS Code will be fixed 🙂

xsebek · 2022-10-19T11:23:07Z

src/Swarm/Game/Step.hs

+                    E.PosInfinity -> replicate 42
+                    E.NegInfinity -> replicate (-42)


Ooops, this is a problem. How should we salvage infinite inventory? 🤔

On IRC we agreed that it should take 1 tick. The idea is that an infinite item is like an item generator so we move it like it had count 1.

There is still an issue with how to do that. I think some "game state update" on the stack of the reprogrammed salvaged robot could work. An internal giveInfinite function also makes sense.

byorgey · 2022-10-22T01:46:33Z

Something doesn't look right:

byorgey

The only code I haven't yet gone over carefully is the inventory manipulation code in Game.Entity.

byorgey · 2022-10-22T01:54:13Z

src/Swarm/Game/CESK.hs

@@ -263,7 +263,7 @@ data CESK
    Up Exn Store Cont
  | -- | The machine is waiting for the game to reach a certain time
    --   to resume its execution.
-    Waiting Integer CESK
+    Waiting Number CESK


Why should this be a Number? It's not useful for this to be infinity, is it? Number is used to represent counts of things, but I don't see why it should also be used to represent times.

Oh, wait, I think I get it --- Number is the Haskell type that is used internally to represent Swarm values of type int. So since wait : int -> cmd unit, therefore Waiting has to take a Number because wait infinity is now valid Swarm code. Did I understand it right?

Yes, it might be less confusing for us to rename int to number.

Besides, it will eventually be useful to wait indefinitely and get woken up by some other robot.

byorgey · 2022-10-22T02:04:30Z

src/Swarm/Game/Entity.hs

@@ -450,9 +452,7 @@ entityInventory = hashedLens _entityInventory (\e x -> e {_entityInventory = x})
 -- Inventory
 ------------------------------------------------------------

-- | A convenient synonym to remind us when an 'Int' is supposed to
--   represent /how many/ of something we have.
-type Count = Int


We could still keep this comment but just change Int to Number.

Ups, I should have kept that. 👍

byorgey · 2022-10-22T02:05:55Z

src/Swarm/Game/Entity.hs

+hashCount :: Num p => Number -> p
+hashCount c = case c of
+  PosInfinity -> 1
+  Integer a | a >= 0 -> 1 + fromIntegral a


Doesn't this mean PosInfinity and Integer 0 both turn into 1? Maybe I'm not understanding what this function is for.

Ah, I think I originally had 2 + and then mistakenly changed that. That will be a pain to adjust again. 🙁

To be clear, I don't yet fully understand how this is used so it might be correct.

byorgey · 2022-10-22T02:06:20Z

src/Swarm/Game/Entity.hs

-fromElems = foldl' (flip (uncurry insertCount)) empty
+fromElems = insertInventoryList empty
+
+insertInventoryList :: Inventory -> [(Count, Entity)] -> Inventory


Let's add a Haddock comment.

byorgey · 2022-10-22T02:17:00Z

src/Swarm/Game/Step.hs

-  In (TRequireDevice {}) e s k -> return $ In (TConst Noop) e s k
-  In (TRequire {}) e s k -> return $ In (TConst Noop) e s k
+  In TRequireDevice {} e s k -> return $ In (TConst Noop) e s k
+  In TRequire {} e s k -> return $ In (TConst Noop) e s k


Did fourmolu insist on getting rid of these parens? I understand intellectually why they are not needed, but I kinda hate it. In TRequire {} e s k really looks like In is taking 5 parameters.

My HLint does not like it and I got tired of it mentioning it. I will change it to (TRequire _num _ent).

We can also turn off specific hlint hints in .hlint.yaml.

In general, I think it is useful to remove extra parenthesis, but this is an edge case.

Presumably, it is different between some HLint versions because CI did not warn about it.

byorgey · 2022-10-22T02:24:08Z

src/Swarm/Game/Step.hs

@@ -1627,13 +1641,13 @@ execConst c vs s k = do
            missingParentInv = neededParentInv `E.difference` parentInventory
            missingMap =
              M.fromList
-                . filter ((> 0) . snd)
+                . filter (\(n, x) -> x > 0 && (x /= PosInfinity || countByName n parentInventory /= PosInfinity))


Can you walk me through the logic here? I'm not sure I follow.

To be safe removing an infinite count of items from an infinite count of items is still an infinite count of items.

But here we were being smart by removing the robot inventory from required items.

So the infinity stays and is not 0.

It is a hack to keep the changes minimal, but we should eventually clean up the code by changing the data structures.
I will write my thoughts on that in a separate comment.

src/Swarm/Game/Step.hs

byorgey · 2022-10-22T02:28:30Z

src/Swarm/Game/Step.hs

+safeExp :: Has (Throw Exn) sig m => Number -> Number -> m Number
+safeExp = \case
+  PosInfinity -> \b -> return $ PosInfinity * signum b
+  NegInfinity -> \b -> return $ NegInfinity * signum b


infinity ^ 0 ought to be 1, not 0.

Well spotted. 👍

byorgey · 2022-10-22T02:30:06Z

src/Swarm/Game/Step.hs

                . map (swap . second (^. entityName))
                . E.elems
                $ missingParentInv

        -- If we're missing anything, throw an error
-        E.isEmpty missingParentInv
+        null missingMap


Why did this change from checking missingParentInv to checking missingMap? Was this a bug before?

I am not sure. But if I hack in the infinity check above, I also need to change this.

The inventory version is likely smart and will ignore 0. See my future comment about the overall data structure.

src/Swarm/TUI/View.hs

xsebek · 2022-10-22T12:52:28Z

My comments on the inventory representation

data Inventory = Inventory
  { counts :: IntMap (Count, Entity)
  , -- ...

While updating the code to work with infinities, I came to the conclusion that this is a problem.

In the infamous words of every programmer: "It worked."

But there are issues with this representation:

it allows an invalid state - count should never be negative
it is abused to also work as known:: Set Entity (or IntSet if we use hash) which causes us to do strange things when counting the hash
the stored entity reference is duplicating information that is known globally
(new) with infinite count the logic of subtraction does not hold

Doubtful PROs of current representation

Keeping this machinery working is beyond my mental capabilities and I am not sure what are its benefits.

Distributed entity knowledge

From the look of the code, I guess that it tries to solve the future issue of robots having entities that are not known globally.
At this point, I think it might have been premature.

Global knowledge of all entities is easier to reason about. In particular, it would allow us to detect hash collisions that might go unnoticed with distributed knowledge of entities between robots.

Performance

I hope that this was never a serious factor in the design. We do not measure the performance, so this could easily be slower and less memory efficient than using different representations designed to be easier to understand.

It fit the code somewhere

At some time there was probably a place in the code where the current representation made it easier to write.

That reason sounds good to me. Is it true still? The hacks that were required for checkRequirements suggest otherwise.

KISS representation

I did not want to change the representation here originally, but updating the hashing once more sounds worse.

I propose a new representation that my simple mind could handle:

global GameState will have all entities in its EntityMap
robots will have a special field for known entities of type IntSet using entity hashes
global GameState will have known entities also stored as IntSet (currently it is [Text])

inventory will have a separate map for infinities and for normal counts:

data Inventory = Inventory
  { finiteCounts :: IntMap Natural
  , infiniteCounts :: IntSet
  , inventoryHash :: Int
  }

item reaching a zero count causes it to be deleted from the map

Hashing will stay homomorphic, but it will separately hash the infinities, the known items and the item counts.

BONUS: Containers should not have the ability to know items. That does not make sense and this will make it impossible.

BONUS2: It should be simple to write hashing function for a list of entity counts, so we can construct inventory using standard Map.fromList instead of folding.

Hash function

I also have a small problem with hash returning precomputed inventoryHash for inventory, but computing the hash anew for the entity even though we store the hash.

Ideally, I would like the behaviour to be the same and use hash e instead of (e ^. entityHash).

BONUS: All hashes should have type Hash, i.e. Word. EDIT: Nvm. that type is internal.

Co-authored-by: Brent Yorgey <byorgey@gmail.com>

byorgey · 2022-10-22T13:39:38Z

@xsebek thanks for writing that up! I love it, and I think that will make a lot of code simpler and solve many of our issues with this PR.

xsebek · 2024-09-10T10:58:28Z

@byorgey, I would like to revive this next, but I might need help. 😄

As far as I know, the points in my final summary are still valid. I will see if they can be split into smaller issues.

byorgey · 2024-09-10T11:25:11Z

I would like to revive this next, but I might need help. 😄

Yes, please do! 🎉 According to my current thinking this is a prerequisite for #231 . I will try to read through this again soon and see how I might be able to help.

restyled-io bot mentioned this pull request Aug 31, 2022

Restyle WIP: Infinite count #657

Merged

byorgey reviewed Sep 1, 2022

View reviewed changes

src/Swarm/Game/Entity.hs Outdated Show resolved Hide resolved

src/Swarm/Game/Entity.hs Outdated Show resolved Hide resolved

src/Swarm/Game/Entity.hs Outdated Show resolved Hide resolved

src/Swarm/Game/Step.hs Outdated Show resolved Hide resolved

xsebek mentioned this pull request Sep 17, 2022

Location watch command #687

Closed

xsebek force-pushed the infinity branch from c946569 to 0b4bf17 Compare October 18, 2022 16:56

xsebek added 9 commits October 18, 2022 19:25

Add Number type with both infinities

057adee

Let Number prolify through the codebase

a5aad65

Fix typo

aae296c

Fix unit tests

8284a59

Fix accidental infinite loop

437f470

Handle addition failure correctly

4676a24

Fix tests expecting old behavior

d4b325c

Restyle with Fourmolu

94f47e8

xsebek force-pushed the infinity branch from 0b4bf17 to 94f47e8 Compare October 18, 2022 17:25

xsebek added 8 commits October 18, 2022 22:34

WIP: Fix the hashing for inventories

656b785

Remove redundant import

47871b1

Fix hashing

bd3f757

Add infinity constant

c36368a

Fix error message

b20cbca

Add unit tests for infinity

450bbda

Use infinity in JSON

e4df1a4

Use infinity in entity requirements

0cdd6ba

xsebek added 4 commits October 19, 2022 12:37

Hack around infinite requirement

5d10af5

Test infinite requirement

a8da1ce

Restyle

0cc6ddc

Apply some hints

6c46bc3

xsebek marked this pull request as ready for review October 19, 2022 11:20

xsebek requested a review from byorgey October 19, 2022 11:21

xsebek commented Oct 19, 2022

View reviewed changes

xsebek changed the title ~~WIP: Infinite count~~ Allow infinite count of items Oct 21, 2022

byorgey reviewed Oct 22, 2022

View reviewed changes

xsebek and others added 2 commits October 22, 2022 15:32

Fix typo

4c02e2e

Co-authored-by: Brent Yorgey <byorgey@gmail.com>

Fix typo

21b075c

Co-authored-by: Brent Yorgey <byorgey@gmail.com>

xsebek added 3 commits October 23, 2022 13:59

Pretty print count in recipes

fec409e

Fix exponentiation of infinity

00d2ba2

Make require pattern readable again

965dfe6

restyled-io bot mentioned this pull request Oct 23, 2022

Restyle Allow infinite count of items #777

Open

xsebek mentioned this pull request Oct 5, 2023

Evaluate constant subexpressions #1570

Open

xsebek mentioned this pull request Oct 2, 2024

Infinite items in inventory #621

Open

		E.PosInfinity -> replicate 42
		E.NegInfinity -> replicate (-42)

Allow infinite count of items #656

Are you sure you want to change the base?

Allow infinite count of items #656

Conversation

xsebek commented Aug 31, 2022 • edited Loading

byorgey commented Sep 1, 2022

byorgey commented Sep 4, 2022

byorgey commented Sep 4, 2022

xsebek commented Sep 5, 2022

xsebek commented Sep 5, 2022

byorgey commented Sep 6, 2022

xsebek commented Oct 18, 2022 • edited Loading

xsebek commented Oct 18, 2022

Choose a reason for hiding this comment

xsebek Oct 22, 2022 • edited Loading

Choose a reason for hiding this comment

byorgey commented Oct 22, 2022

byorgey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byorgey Oct 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xsebek commented Oct 22, 2022 • edited Loading

My comments on the inventory representation

Doubtful PROs of current representation

Distributed entity knowledge

Performance

It fit the code somewhere

KISS representation

Hash function

byorgey commented Oct 22, 2022

xsebek commented Sep 10, 2024

byorgey commented Sep 10, 2024

xsebek commented Aug 31, 2022 •

edited

Loading

xsebek commented Oct 18, 2022 •

edited

Loading

xsebek Oct 22, 2022 •

edited

Loading

byorgey Oct 22, 2022 •

edited

Loading

xsebek commented Oct 22, 2022 •

edited

Loading