joostberkhout / pykda Goto Github PK

Hé @JoostBerkhout, ik dacht het is wel zo makkelijk om feedback op Github te doen - dan kan ik ook refereren naar code en kunnen we dit interactiever bespreken.

Docs

• Leuk logo! Logo kun je het best vervangen van JPG naar PNG. JPG kan geen transparante achtergronden hebben, maar PNG wel. Zodat je geen white background hebt in dark mode
• Je kunt overwegen om een CITATION.cff file toe te voegen aan je repository. Zie https://github.com/N-Wouda/ALNS/blob/master/CITATION.cff. Dan krijg je citation data in de sidebar.
• Je kunt ook je examples renderen in je documentatie en tegelijkertijd beschikbaar maken in colab.
  • https://github.com/danielfrg/mkdocs-jupyter : dit is een package om je notebooks te renderen in mkdocs.
  • Hier moet je even voor googlen maar je kunt volgens mij heel makkelijk een Colab link maken door deze string te formatten naar jouw eigen situatie: https://colab.research.google.com/github/username/repository/blob/master/notebook.ipynb
    Die blijft dan ook altijd up-to-date met veranderingen op je Github.
  • Om je notebook lokaal en op Colab te runnen heb je een beetje extra boilerplate nodig, zie voorbeeld van Alessandro's boek: https://colab.research.google.com/github/mobook/MO-book/blob/main/notebooks/02/03-mad-portfolio-optimization.ipynb#scrollTo=5ssUqKOaPVaE In het eerste code block wordt gecheckt of je in een colab environment zit, en zoja, dan pip install je de package. Lokaal wordt dit genegeerd.

Code

• Style:
  • Ik zou Markov_chain.py veranderen naar MarkovChain.py or markov_chain.py. Eerste is PascalCase' stijl en tweede is Python stijl. De huidige file name zit er tussenin en valt buiten de conventies.
• Typing:
  • np.ndarray or list[list] or str -> Union[np.ndarray, list[list], str] of je kunt | gebruiken

    • PyKDA/pykda/Markov_chain.py

      Line 30 in 166c6e4

      P : np.ndarray or list[list] or str

  • Vanaf Python 3.9 kun je list gebruiken in plaats van typing.List. Zelfde met dict en tuple.

    • PyKDA/pykda/Markov_chain.py

      Line 7 in 166c6e4

      from typing import List, Tuple

  • -> None output kun je weglaten, dat is namelijk impliciet

    • PyKDA/pykda/Markov_chain.py

      Line 37 in 166c6e4

      def __init__(self, P: np.ndarray | List[List] | str) -> None:

• Features
  • Heb je cached_property echt nodig op je MarkovChain class? Ik ken je algoritme niet dus ik weet niet hoe vaak bepaalde properties worden aangeroepen. Maar MarkovChain heeft veel internal state en lijkt redelijk vaak te updaten. cached_property cachet een property, maar als je interne variabelen veranderen, dan wordt dit dus niet meer geupdated. Een robustere design keuze is om niet te cachen en om property te gebruiken.

    • PyKDA/pykda/Markov_chain.py

      Line 65 in 166c6e4

      @cached_property

  • normalizer argument wordt niet gebruikt in load_predefined_transition_matrix
    • In normalizer.py kun je normalizer_type = Callable[[np.ndarray], np.ndarray] het best bovenaan zetten (is meteen duidelijk) en hebt best met _ prefix odat je daarmee niet perongeluk user variables overschrijft. Dus _normalizer_type.

    • PyKDA/pykda/normalizers.py

      Line 95 in 166c6e4

      normalizer_type = Callable[[np.ndarray], np.ndarray]

  • Het KDA algoritme neemt nu als input een markov chain en slaat deze op, en past dit telkens aan als je run aanroept. Een robuustere interface is als volgt: je slaat de MC chain niet op, maar geeft dit mee als argument in run. Als output krijg je je nieuwe MC terug. Hetzelfde kun je doen met cut_edges.

    • PyKDA/pykda/KDA.py

      Line 43 in 166c6e4

      original_MC: MarkovChain,

Add to the testing workflow on Github that it is also tested in Python 3.12.