Implement Djikstra in CCXT Pair Converter

[macanudo527]

This is the implementation of the Djikstra algo in the CCXT Pair Converter.

I'd like some feedback on the weights. I think these are fair, but if you could imagine a scenario where these don't work let me know.

I'm also wondering about vertexes. I pop them out of Graph with a list comprehension in each function, but I'm guessing this should be a function somewhere. Should it be a class method in ccxt.py or should we push it down into prezzemolo? I'm guessing class method.

This is WIP. I should run it with real-world data, which will take some time, but if all goes well, it should finish over the weekend.

[macanudo527]

I'm not sure how to proceed. I'm not getting this error locally.

I have python 3.10.6 in my virtualenv, but the CI is running 3.8.16. I think that is what is causing it. Is there a way to safely switch python versions, or a way to make mypy happy at the lower version?

[eprbell]

Looks like you're subclassing defaultdict at the line where the error is occurring. Subclassing Python system classes could be tricky. I suggest using composition rather than inheritance:

• if you want to have a class (i.e. GraphVertexesDict) to encapsulate your Vertexes, don't subclass dict, but simply add a dict to the class as a field. You can also add methods to the class to add and get vertexes; Or
• add the dict directly to AbstractPairConverter (as a private or protected variable) and add protected methods to read from / write to it.

My personal preference would be for the second method (no extra class), but it's up to you.

As for dropping 3.7: I agree, I think we should drop 3.7 and add 3.11.

[macanudo527]

Maybe I'm just being too fancy for my own good. The whole point was to add to Graph whenever the defaultdict found a missing Vertex all in one function. I was trying to avoid conditionals. I'll just have to use them I guess.

[eprbell]

Maybe I'm just being too fancy for my own good. The whole point was to add to Graph whenever the defaultdict found a missing Vertex all in one function. I was trying to avoid conditionals. I'll just have to use them I guess.

You can still do the same with the following approach. Add the following to AbstractPairConverterPlugin:

• __vertexes: the dictionary
• _get_vertex(name)
• _add_vertex(name, vertex)
• _get_or_set_vertex(name, vertex): equivalent to setdefault
• whatever else you may need...

What conditionals are you referring to?

[macanudo527]

Maybe I'm just being too fancy for my own good. The whole point was to add to Graph whenever the defaultdict found a missing Vertex all in one function. I was trying to avoid conditionals. I'll just have to use them I guess.

You can still do the same with the following approach. Add the following to AbstractPairConverterPlugin:

• __vertexes: the dictionary
• _get_vertex(name)
• _add_vertex(name, vertex)
• _get_or_set_vertex(name, vertex): equivalent to setdefault
• whatever else you may need...

What conditionals are you referring to?

Basically, I'll have to recode defaultdict. Psuedo-code:

self.__vertexes.get(current_vertex) # This is already built into defaultdict
if not current_vertex:                     # This line, too
    current_vertex = Vertex[str](name="name")
    current_graph.add_vertex(current_vertex)
   self.__vertexes["name"] = current_vertex

I was trying to overload defaultdict since it is already in there, and it works. Just Python 3.7.16 doesn't seem to recognize mypy type declarations in class inheritance.

These methods will have to be added to a data class. If they are added to AbstractPairConverterPlugin the graph would have to be passed to each method. Currently, each exchange has its own graph. I'd like to implement one universal graph, but that will take a little more architecture. Like I said, I was trying to be too fancy.

[macanudo527]

My mistake was that I was using defaultdict from collections and I should have been using DefaultDict from Typing which is capable of taking typing. I can use defaultdict with Python 3.9+, but pre-3.9 you need to stick to the Typing classes. Go figure.

This is no longer WIP. It has been tested with real-world data and came up with matching results, so it should be ready to go.

I added a test for GraphVertexesDict which will hopefully illuminate what exactly it does. Let me know if it is a little too 'magical' and there should be something more explicit, but I think I documented it well enough that other devs should know what is going on.

[macanudo527]

I hope this is what you were thinking. I added a function add_vertex_by_name() since this class works with names.

I've been trying to make a full run off and on. I should be able to finish it in the next 12 hours or so.

[macanudo527]

This is good to go with real-world data. I did spot a bug with BETH pricing #143 that I'll have to address later, but it existed before this update.

[eprbell]

I hope this is what you were thinking. I added a function add_vertex_by_name() since this class works with names.

I've been trying to make a full run off and on. I should be able to finish it in the next 12 hours or so.

What I meant was to override add_vertex():

    def add_vertex(self, vertex: Vertex[ValueType]) -> None:
        super().add_vertex(vertex)
        self.__name_to_vertex[vertex.name] = new_vertex

The add_vertex_by_name() is not mandatory in my opinion, but if you find it useful in your code you can add it.

[eprbell]

This is good to go with real-world data. I did spot a bug with BETH pricing #143 that I'll have to address later, but it existed before this update.

Cool, I'll review soon: I've been a bit swamped lately.

[macanudo527]

This is good to go with real-world data. I did spot a bug with BETH pricing #143 that I'll have to address later, but it existed before this update.

Cool, I'll review soon: I've been a bit swamped

I hope this is what you were thinking. I added a function add_vertex_by_name() since this class works with names. I've been trying to make a full run off and on. I should be able to finish it in the next 12 hours or so.

What I meant was to override add_vertex():

    def add_vertex(self, vertex: Vertex[ValueType]) -> None:
        super().add_vertex(vertex)
        self.__name_to_vertex[vertex.name] = new_vertex

The add_vertex_by_name() is not mandatory in my opinion, but if you find it useful in your code you can add it.

That actually makes more sense. I guess I wasn't thinking.

[macanudo527]

Did something need to be added to this @eprbell ?