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

WIP: Minor code improvements #67

Closed
wants to merge 11 commits into from
Closed

WIP: Minor code improvements #67

wants to merge 11 commits into from

Conversation

Solomon1732
Copy link
Contributor

@Solomon1732 Solomon1732 commented Feb 23, 2021

Implement minor code improvements as discussed in #63
Fixes #63

@tribler-ci
Copy link

Can one of the admins verify this patch?

@devos50
Copy link
Contributor

devos50 commented Feb 23, 2021

ok to test

@devos50
Copy link
Contributor

devos50 commented Feb 23, 2021

add to whitelist

@devos50
Copy link
Contributor

devos50 commented Feb 23, 2021

retest this please

@Solomon1732
Copy link
Contributor Author

Solomon1732 commented Feb 24, 2021

Seems like I can't test it locally. It keeps failing on import of orjson. Even when I install the package it fails importing it when I run the tests.

@devos50
Copy link
Contributor

devos50 commented Feb 24, 2021

That's unexpected. Doing a pip install orjson should work. Maybe you could try to install the package both in user space and system-wide?

@Solomon1732
Copy link
Contributor Author

Solomon1732 commented Feb 24, 2021

I just had to uninstall the module than reinstall it. I have no idea what happened before. I'm getting different errors now, but at least I've solved that problem.

Edit: These are the errors I'm getting: https://gist.github.com/Solomon1732/d079b53b701aceabd2cffb3f47195e5a

Solomon1732 added 11 commits February 26, 2021 16:23
Use an emptry call to `super()` instead of hardcoding the arguments.
Also use str.format instead of the % operator. This was chosen instead
of f-strings since it allows to use the equivalent of %g.
Modified loop structures in some functions. Instead of constructing the
lists one element at the time, iterators, genexprs, and list comprehensions are used in order to make the list building more efficient.
Modified functions' internal list handlings. Instead of using
temporary lists or building lists by repeatedly appending items, the
modified functions now use list comprehensions, `list.extend`,
genexps, and iterators to more efficiently build lists.

Removed some declarations of `object` as parent class.

Modified `PriceLevelList.insert` to use `bisect.insort` to avoid having
to sort the list at each item insertion.

Replace explicit super() calls with no args calls

Replaced explicit calls to `super()` with either the no arguments
version or outright deleted.

Minor refractoring to reduce code reduplication

Minor refractoring to reduce code reduplication.
Changed % string operator occurances to f-string when possible, or to
`str.format` when not.
Changed the structure of a few functions, namely orderbook's __str__
function. Reduces code duplication. Also changed _get_order_ids_iter and
_get_price_level_iter.
Removed `object` as parent class from various classes. This is because
it is redundant in python 3.
Changed function to use list comprehention instead of appending to the
list one element at a time.

Change how some functions work

In `BlockCache.get_missing_in_range` used list comprehention instead of
manually appending to a new list in a loop.

In `OrderBook._get_order_ids` used `map` instead of manually yielding
in a loop.

In `MatchPriorityQueue` used a `namedtuple` as the element of the internal
`queue` list. Used `any` with a generator expression in `contains_order`
instead of the loop with `_, _, other_order_id, _` destructuring,
utilizing the introduced `namedtuple`. In `insert` utilized the new
`namedtuple` in the internal `cmp_items` function for maintainability
and readability. Also in `insert` instead of creating a new sorted list
for `self.queue` the queue is sorted in-place.
Used `next` with a default value of `None` instead of using `try-except`
on `StopIteration`.

Removed `u` prefix from string. The prefix is superfluous in python 3.

Instead of checking `current_version` against individual numbers it is
checked against a tuple of values.

Change parameterized `super()` to empty `super()`

Instead of calling `super()` with explicit parameter, an empty call to
it is made instead.
Changed the raising of ValueError to TypeError when appropriate. Namely
when an argument's type is checked. Likewise changed the tests to match
the new exceptions thrown.

In `bitcoinlib_main.py` moved some software structures out of a file's
`with` statement in order to not hold the file open longer than
necessary. In addition restructured a for-loop in a minor way in order
to decrease the indentation level of the loop's body.
Modified PriceLevelList to utilize bisect for various methods. In
addition, modified the class to raise IndexError manually in some cases
so as to hide implementation details.
Added a comment to improve code readability and highlight the difference
between the two if cases.

Also made the two cases one-liners.
@Solomon1732
Copy link
Contributor Author

Latest commit was mainly to add a comment in order to prevent further issues like #68 being opened in the future upon code inspection.

@Solomon1732
Copy link
Contributor Author

@devos50 in match_queue I'm trying to implement the internal items as a NamedTuple (from python's typing). However when trying to write the methods for less-than, greater-than, equal, and so forth, I don't know how to compare the internal structure of the tuple in such a way that it would result in the desired order. Could you maybe advise me as to what items I should compare? I just don't want to blindly copy the cmp_items method over and over again, with little modifications only as the differences between the rich-comparison methods (lt, ge, eq, etc.)

Currently the named tuple is this:

    @dataclasses.dataclass(eq=True, frozen=True)
    class _QueueItem(NamedTuple):
        retries: int
        price: Price
        is_ask: bool
        order_id: OrderId
        other_quantity: Any

@devos50
Copy link
Contributor

devos50 commented Feb 26, 2021

Good question. First, you should understand some basics of the matchmaking mechanism as implemented in AnyDex. In AnyDex, matchmakers send match notifications to traders that have outstanding buy and sell orders. When a trader receives such a notification, it adds an entry to the match queue of the associated order. The match queue is a priority queue that prioritizes orders with a high quality. Each entry in the match queue has a few fields as you implemented in the _QueueItem data class. The retries field is an integer that indicates how many times we have tried to negotiate about a specific entry (also see Figure 3 in our article).

To answer your question, we prioritize first on retries, and then on the price (a better price will be prioritized). However, what constitutes the 'better' price depends on whether the order is a buy or sell order. This is the logic on line 33-42 in the match queue logic.

Hopefully this provides sufficient information for you to continue 👍

@Solomon1732
Copy link
Contributor Author

So, just to see if I'm on the mark, is this example fitting for what you have in mind?

def __eq__(self, other) -> bool:
    return self.retries == other.retries and self.price == other.price

def __lt__(self, other) -> bool:
    if self.retries < other.retries:
        return True
    elif self.retries == other.retries:
        if self.match_order.is_ask():
            return self.price < other.price
        else:
            return not (self.price < other.price)
    else:
        return False

@devos50
Copy link
Contributor

devos50 commented Feb 27, 2021

@Solomon1732 I think it is the correct logic, but you could consider adding a few unit tests to more thoroughly check/compare your solution 👍

@devos50
Copy link
Contributor

devos50 commented Apr 12, 2021

@Solomon1732 any plans to finish this PR?

@Solomon1732
Copy link
Contributor Author

Things happened and I completely forgot about this PR! I'm so sorry! I'll close it for now since in the near future I don't think I'll manage to push this through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible minor code improvements
3 participants