krukah / robopoker

Play, learn, solve, and analyze No-Limit Texas Hold Em. Implementation follows from Monte Carlo counter-factual regret minimization over with hierarchical K-means imperfect recall abstractions.
MIT License
43 stars 4 forks source link

Metrics never set #21

Closed reinismu closed 1 month ago

reinismu commented 1 month ago

Seems like there is a bug that metrics are never set with values. Now when running it will fail

07:38:19 - resetting abstractions Turn 98
07:38:19 - computing abstractions Turn 99
07:41:05 - assigning abstractions Turn 99
07:41:11 - resetting abstractions Turn 99
07:41:11 - uploading abstraction metric Turn.abstraction.pgcopy
07:41:11 - uploading abstraction lookup table Turn.abstraction.pgcopy
07:41:22 - progress: 12s         2921688  10.00%   mean 247039   last 247039
07:41:34 - progress: 24s         5843376  20.00%   mean 247101   last 247162
07:41:46 - progress: 35s         8765064  30.00%   mean 247416   last 248050
07:41:58 - progress: 47s        11686752  40.00%   mean 247704   last 248572
07:42:09 - progress: 59s        14608440  50.00%   mean 247897   last 248671
07:42:21 - progress: 71s        17530128  60.00%   mean 248047   last 248800
07:42:33 - progress: 82s        20451816  70.00%   mean 248129   last 248625
07:42:45 - progress: 94s        23373504  80.00%   mean 248164   last 248410
07:42:56 - progress: 106s       26295192  90.00%   mean 248262   last 249045
07:43:08 - progress: 118s       29216880 100.00%   mean 248374   last 249384
07:43:08 - advancing from Turn to Flop
07:43:08 - computing metric Turn
07:43:08 - computing projections Turn
07:43:09 - initializing kmeans Flop
07:43:09 - add initial 1
thread '<unnamed>' panicked at src/clustering/metric.rs:43:18:
precalculated distance
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at src/clustering/metric.rs:43:18:
precalculated distance
thread '<unnamed>' panicked at src/clustering/metric.rs:43:18:
precalculated distance
thread '<unnamed>' panicked at src/clustering/metric.rs:43:18:
precalculated distance

Run it using my shortdeck branch to test easier

krukah commented 1 month ago

have seen, can reproduce – feels like it should be obvious to me looking at the code but scratching my head on this one. i think there's a mistake with the order in which `self.

although, i realize in an earlier refactor i may have changed the metric calculation to handle the edge case of earth-mover distance where we try to lookup a "degenerate" pair, i.e. Pair::from(x, x).

this might fix? https://github.com/krukah/robopoker/commit/fa60546e35faf266744fac6bf54c8325ebab5f53

reinismu commented 1 month ago

From what I can tell when compute gets till fn distance(&self, x: &Abstraction, y: &Abstraction) -> f32 { Metric BTree is empty. image

I don't thing setting default Pair will fix it. From what I can tell it needs to contain many different keys.

Seems that issue is in code order. My best guess is that need to change order of metrics and points here

 let mut layer = Self {
            lookup: Abstractor::default(),       // assigned during clustering
            kmeans: AbstractionSpace::default(), // assigned during clustering
            street: self.inner_street(),         // uniquely determined by outer layer
            metric: self.inner_metric(),         // uniquely determined by outer layer
            points: self.inner_points(),         // uniquely determined by outer layer
        };

So metrics would be set when kmeans are calculated. Tho best would be to rewrite how this is handled. Currently it is not obvious that order is important there and can cause bugs as this one in the future as well

krukah commented 1 month ago

IMG_90E66B189366-1

it's true that it's not obvious, but order should not be important here if i've implemented my mental model correctly. the idea is that some of the Layer properties (metric & street & abstractionProjections) fields are computed as a pure function of the previous layer (via inner[_points, _metric, _street] via &self)

the base case of the outermost River layer is a bit odd because it has all of these as empty

the 1st case Turn is a bit less odd, but it still has an empty metric bc we can calculate its histogram over the real numbers, and we don't have to use learned kmeans distances, yet

but Metric handles that edge case internally, differentiating between the abstract wasserstein distance vs the scalar difference distnace -- not so well named, admittedly

krukah commented 1 month ago

so my best guess as to what was happening earlier was that the Pair from 0u64 entry was missing, violating the assertion that distance(x, y) would be defined for any x, y: Abs , since we are certain to encounter the case of x == y => Pair(x,y) == Pair::from(0) which is not in the index of the Metric

krukah commented 1 month ago

can confirm this missing diagonal zero entry was the problem, was able to encounter a new fatal error that's a trivial fix – just have to deprecate Progress in favor of 3rd party progress bar image