Test comparable to combinatorial auction filtering #80

fhenneke · 2023-10-26T08:20:14Z

We are thinking about implementing combinatorial auctions. This would include filtering out solutions which do not provide more surplus on individual sell token-buy token pairs compared to solutions only including that token pair.

The test in this PR checks if the winning solution would have been filtered out based on this criterion.

One difference to the competition surplus test is that the test in this PR checks token pairs and not orders.

Potentially we want to check more in this test. E.g.

Who would have won the auction with the filtering rule?
Who would have won the auction with the option to declare multiple winners?
What would the benefit in surplus have been?

Todo

Fix relative test to compare price improvement and not surplus improvement.

src/monitoring_tests/combinatorial_auction_surplus_test.py

fhenneke · 2023-10-26T13:25:01Z

I significantly changed the code to really run a simple combinatorial auction.

This gives slightly different information than previously: Before I logged which orders would have provided more surplus on token pairs and would have resulted in filtering the top solution. Now, I log who would have won the combinatorial auction and how much more surplus would have been provided. Both information is useful so I might add the old stuff back. I want to find a cleaner way to do that though. Since the old information is close to some other competition surplus test I went for the code as in the PR.

- aggregate orders on token pairs - compare winning solution to all solutions with just one order

- computes winners using combinatorial auction - change in winner implies top solution was filtered out - logs change in surplus - removes old test of just comparing the winner to other solutions

- add more debugging info - show which solutions are filtered and why - only use objective instead of surplus for single token pair solutions - make naming consistent: `aggregated_solutions` -> `aggregate_solutions` - add more test cases

harisang · 2023-11-08T16:12:52Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+
+
+class CombinatorialAuctionSurplusTest(BaseTest):
+    """Test how a combinatorial auctions would have settled the auction.


Plural (auctions) not needed in the comment

harisang · 2023-11-08T16:13:37Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+    - Aggregate surplus on the different directed sell token-buy token pairs for all solutions in
+      the competition.
+    - Compute a baseline for surplus on all token pairs from those solutions.
+    - Filter solutions which to not provide at least as much surplus as the baseline on all token


Typo: "to not" -> do not.

Also, phrase "on all token pairs" could be read as follows: a solver should deal with all token pairs and provide the most surplus in all of them. Maybe add "on all token pairs that the solution touches" ?

harisang · 2023-11-08T16:36:42Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+        """
+
+        solutions = competition_data["solutions"]
+        solution = competition_data["solutions"][-1]


Maybe rename to winning_solution?

It would also help to not confuse things with the for-loop variable in line 60

harisang · 2023-11-08T16:52:16Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+                    int(prices[token_pair[0]]),
+                    10**36,
+                )
+            elif trade.get_surplus_token() == trade.data.buy_token:


is it possible there is a third case?

No. Should I rewrite as else instead of elif? Or add some error message in case both checks fail?

I rewrote this as if "is sell order": ... else:

harisang · 2023-11-08T21:49:07Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+            )
+            for solution in solutions
+        ]
+        aggregate_solution = aggregate_solutions[-1]


Same here, a renaming of the variable would help.

harisang · 2023-11-08T22:25:35Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+                surplus_dict[token_pair] = min(
+                    surplus_dict[token_pair], solution["objective"]["total"]
+                )
+                # surplus_dict[token_pair] = solution["objective"]["total"]


let's remove this comment

harisang · 2023-11-08T22:50:38Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+        result: dict[tuple[str, str], tuple[Fraction, int]] = {}
+        for i, aggregate_solution in enumerate(aggregate_solutions):
+            if len(aggregate_solution) == 1:
+                token_pair, surplus = list(aggregate_solution.items())[0]


This and the next line are not the easiest to read but they make sense.

I added some comments to make it easier to understand. Still a bit messy since I am using (surplus, index) tuples.
A simpler design would be nice.

harisang · 2023-11-08T23:02:57Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+    ) -> list[list[int]]:
+        """Filter out solutions which do not provide enough surplus.
+        Only solutions are considered for ranking that supply more surplus than any of the single
+        aggregate order solutions. The function returns a list of bools where False means the


The comment about what the function returns is not accurate.

Also, maybe we could make what the function returns easier to understand?

I fixed the comment and added some rational for making it this complicated.
A simpler design would be nice though.

harisang · 2023-11-09T11:53:21Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+                "Combinatorial auction surplus test:",
+                f"Tx Hash: {competition_data['transactionHash']}",
+                f"Winning Solver: {solution['solver']}",
+                f"Winning surplus: {aggregate_solution}",


The surplus is a fraction here and so the printing is not easy to read. Would be good to convert this to a decimal in ETH.

I convert this to float now. It looks extremely ugly though. I am a fan of using Fraction in the actual computation though. (I already made a big concession by using ETH and not wei as unit 😜)

harisang · 2023-11-09T11:53:55Z

Tested locally and seems to be working. Will do one more pass to double-check everything but it looks good!

Added a few minor comments here and there.

harisang

Added some minor comments that would be good to be addressed.

- fixed comments - renamed variables - added comments - simplified buy and sell order cases - simplified some default value dict access

just experimental since it looks a bit ugly

also removed unnecessary pylint disable

harisang · 2023-11-10T00:16:05Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+    def run(self, tx_hash: str) -> bool:
+        """
+        Wrapper function for the whole test. Checks if solver competition data is retrievable
+        and runs EBBO test, else returns True to add to list of unchecked hashes.


Runs EBBO test..

Maybe fix this.

else returns True...
is this a typo or is this the convention here?

I fixed it. The same error is present in some other test. Will fix that later.

harisang · 2023-11-10T00:42:17Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+
+        # convert surplus to float for logging
+        winning_aggregate_surplus_float = {
+            token_pair: float(surplus)


Can we maybe reduce the number of decimals to 4? I guess we are not doing it in other tests but it would make things more readable.

Something like round(float_variable,4) should do it.

I added a more general function to process those dicts before logging.

harisang · 2023-11-10T00:43:02Z

Looks good! Added some minor comments, and I guess adding the test in the daemon.py is still missing.

info logs remain the same for a better overview

harisang · 2023-11-10T12:04:33Z

src/monitoring_tests/combinatorial_auction_surplus_test.py

+            result[solver][token_pair] = aggregate_solutions[solution_index][token_pair]
+        return result
+
+    def convert_fractions_to_floats(self, obj: Any, precision: int = 4) -> Any:


harisang · 2023-11-10T12:05:32Z

Things look good. Let's merge and observe!

Can you create a new release btw once this is merged as there 2-3 PRs merged already so a release makes sense.

fhenneke requested a review from harisang October 26, 2023 08:29

harisang reviewed Oct 26, 2023

View reviewed changes

src/monitoring_tests/combinatorial_auction_surplus_test.py Outdated Show resolved Hide resolved

harisang reviewed Oct 26, 2023

View reviewed changes

src/monitoring_tests/combinatorial_auction_surplus_test.py Outdated Show resolved Hide resolved

harisang reviewed Oct 26, 2023

View reviewed changes

src/monitoring_tests/combinatorial_auction_surplus_test.py Outdated Show resolved Hide resolved

harisang reviewed Oct 26, 2023

View reviewed changes

src/monitoring_tests/combinatorial_auction_surplus_test.py Outdated Show resolved Hide resolved

fhenneke added 5 commits November 2, 2023 18:20

initial implementation of a test comparable to combinatorial auctions

8bcfd56

- aggregate orders on token pairs - compare winning solution to all solutions with just one order

changed the test to actually compare to combinatorial auction

7996e85

- computes winners using combinatorial auction - change in winner implies top solution was filtered out - logs change in surplus - removes old test of just comparing the winner to other solutions

change name and doc string of main function for combinatorial auction

5891fda

small update

34625d6

- add more debugging info - show which solutions are filtered and why - only use objective instead of surplus for single token pair solutions - make naming consistent: `aggregated_solutions` -> `aggregate_solutions` - add more test cases

reshuffle code, add more test hashes

b10c702

fhenneke force-pushed the combinatorial_filtering branch from e389306 to b10c702 Compare November 2, 2023 17:21

harisang reviewed Nov 8, 2023

View reviewed changes

harisang marked this pull request as ready for review November 9, 2023 11:51

harisang reviewed Nov 9, 2023

View reviewed changes

harisang requested changes Nov 9, 2023

View reviewed changes

fhenneke added 4 commits November 10, 2023 00:22

addressed review comments

7bfb474

- fixed comments - renamed variables - added comments - simplified buy and sell order cases - simplified some default value dict access

convert fractions to float for logging

ad17d01

just experimental since it looks a bit ugly

fix pylint error

4719e66

also removed unnecessary pylint disable

pyling disable was necessary after all

f49fbee

fhenneke requested a review from harisang November 9, 2023 23:43

harisang reviewed Nov 10, 2023

View reviewed changes

fhenneke added 5 commits November 10, 2023 12:53

changed doc string in run function

1414bad

add function for converting fractions to floats for logging

a2ce374

add test to daemon

d170039

Merge branch 'main' into combinatorial_filtering

9e83d51

removed most alerts

11a39d1

info logs remain the same for a better overview

harisang reviewed Nov 10, 2023

View reviewed changes

fhenneke requested a review from harisang November 10, 2023 12:04

harisang approved these changes Nov 10, 2023

View reviewed changes

fhenneke merged commit bcde774 into main Nov 10, 2023
4 checks passed

fhenneke deleted the combinatorial_filtering branch November 10, 2023 13:14

github-actions bot locked and limited conversation to collaborators Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test comparable to combinatorial auction filtering #80

Test comparable to combinatorial auction filtering #80

fhenneke commented Oct 26, 2023

fhenneke commented Oct 26, 2023

harisang Nov 8, 2023

harisang Nov 8, 2023 •

edited

Loading

harisang Nov 8, 2023 •

edited

Loading

harisang Nov 8, 2023

fhenneke Nov 9, 2023

fhenneke Nov 9, 2023

harisang Nov 8, 2023

harisang Nov 8, 2023

harisang Nov 8, 2023

fhenneke Nov 9, 2023

harisang Nov 8, 2023 •

edited

Loading

fhenneke Nov 9, 2023

harisang Nov 9, 2023

fhenneke Nov 9, 2023

harisang commented Nov 9, 2023

harisang left a comment

harisang Nov 10, 2023

fhenneke Nov 10, 2023

harisang Nov 10, 2023 •

edited

Loading

fhenneke Nov 10, 2023

harisang commented Nov 10, 2023

harisang Nov 10, 2023

harisang commented Nov 10, 2023



		class CombinatorialAuctionSurplusTest(BaseTest):
		"""Test how a combinatorial auctions would have settled the auction.

Test comparable to combinatorial auction filtering #80

Test comparable to combinatorial auction filtering #80

Conversation

fhenneke commented Oct 26, 2023

fhenneke commented Oct 26, 2023

Choose a reason for hiding this comment

harisang Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

harisang Nov 8, 2023 • 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

harisang Nov 8, 2023 • 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

harisang commented Nov 9, 2023

harisang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harisang Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harisang commented Nov 10, 2023

Choose a reason for hiding this comment

harisang commented Nov 10, 2023

harisang Nov 8, 2023 •

edited

Loading

harisang Nov 8, 2023 •

edited

Loading

harisang Nov 8, 2023 •

edited

Loading

harisang Nov 10, 2023 •

edited

Loading