search

HyperLEDA / db-app

Backend for HyperLeda astronomical database of extragalactic objects
https://hyperleda.github.io/db-app/
MIT License
0 stars 0 forks source link

Cross identification use case #142

Closed d-makarov-d closed 2 months ago

d-makarov-d commented 4 months ago

Cross identification use case implementation and testing

d-makarov-d commented 4 months ago

Benchmarks for coss identification of data, that is processed

NPoints Method Init time init mem time mem
1000 simple 1.1842000276374165e-06 s 520 B 0.27270523269999103 s 1591 B
5000 simple 9.747000149218366e-07 s 696 B 1.362887727999987 s 1307 B
10000 simple 9.039000360644422e-07 s 696 B 2.6923894366999774 s 1136 B
20000 simple 9.348000276077073e-07 s 696 B 5.2818602004999775 s 908 B
50000 simple 8.591000550950412e-07 s 696 B 13.299184179299937 s 794 B
100000 simple 8.72999953571707e-07 s 696 B 27.283664147299987 s 851 B
1000 KDTree 0.1397985143000028 s 2496 B 0.009556023200002529 s 648 B
5000 KDTree 0.6949047902999951 s 1464 B 0.04530520970000111 s 568 B
10000 KDTree 1.3894745881999995 s 1464 B 0.09047014139999873 s 568 B
20000 KDTree 2.7816018049999967 s 1464 B 0.18125534010000025 s 4016 B
50000 KDTree 6.960445219999997 s 1464 B 0.4546720884000024 s 568 B
100000 KDTree 13.979613529100003 s 1464 B 0.9161960672000078 s 568 B
1000 PG 0.19790853849997347 s 138917 B 0.005958995499997855 s 127815 B
5000 PG 0.8417563099999938 s 121011 B 0.007068349300004684 s 117354 B
10000 PG 1.6844553614999995 s 118249 B 0.008423732200003541 s 119797 B
20000 PG 3.3574917550000123 s 118118 B 0.011614575800001603 s 128431 B
50000 PG 8.103130566499999 s 117811 B 0.02065682040000638 s 136188 B
100000 PG 16.223209063500008 s 116437 B 0.036517746699996675 s 151651 B
d-makarov-d commented 2 months ago

По возвращаемым эксепшенам - это в отдельную ищью, либо к рефакторингу добавить. Потому что это у меня тянется много откуда, соответственно кучу кода надо менять

Kraysent commented 2 months ago

По возвращаемым эксепшенам - это в отдельную ищью, либо к рефакторингу добавить. Потому что это у меня тянется много откуда, соответственно кучу кода надо менять

Ну я не имею ввиду сразу везде менять, но по крайней мере в коде, который меняется в этом ПРе, можно сразу поменять, сам же себе потом работу облегчишь:

Вот эти функции например и создаются, и используются только в новом коде, тут сразу же можно сделать правильное возвращающее значение

Вот это возвращаемое значение тоже используется только в новом коде, можно сразу сделать правильно

d-makarov-d commented 2 months ago

Ну в целом то надо во всем коде поменять) Потому что если сейчас быстро могу только | на dataclass поменять, но от этого эксепшены не перестанут возвращаться

Kraysent commented 2 months ago

Ну в целом то надо во всем коде поменять) Потому что если сейчас быстро могу только Optional на dataclass поменять, но от этого эксепшены не перестанут возвращаться

Ну сейчас у тебя как будто не много надо сделать

Да и вроде всё, всё остальное без этого сработает, всё остальное можно поменять уже отдельно

d-makarov-d commented 2 months ago

Вообще мне Union больше нравится чем dataclass. Он однозначней, либо то, либо другое. А в такой Result формально можно например два None запихнуть

d-makarov-d commented 2 months ago

Или это некая стандартная практика для питона? Таким образом результат оформлять?

Kraysent commented 2 months ago

Или это некая стандартная практика для питона? Таким образом результат оформлять?

Ну смотри, ответ на две части разобью

  1. Стандартная практика для питона - рейзить ошибки, а не возвращать (как и для кучи других языков, включая джаву и котлин, кажется).

    Тогда код можно переписать в виде

    async def invoke(
    self,
    param: CrossIdentificationParam,
    simultaneous_data_provider: CrossIdSimultaneousDataProvider,
    user_param: CrossIdentificationUserParam,
) -> result.CrossIdentifySuccess:
  if param.names is None and param.coordinates is None:
      raise result.CrossIdentificationEmptyException()
  ....

  return res

    и получится норм с точки зрения практики питона.

  2. Следующее утверждение, уже связанное не конкретно с питоном, а в целом с использованием функции - бросание ошибок, как выше, усложняет читаемость функции, потому что по сигнатуре функции def invoke(self, CrossIdentificationParam, CrossIdSimultaneousDataProvider, CrossIdentificationUserParam) -> result.CrossIdentifySuccess непонятно, какие ошибки она бросает, и соответственно чтобы пользователь функции мог понять, как ей пользоваться, ему нужно залезать в реализацию функции и просмотреть код функции на предмет возможных ошибок. Это часто ведёт к ошибкам из-за человеческого фактора - читатель может не увидеть один из возможных видов ошибок, или, например, при изменении логики добавим новый вид ошибок и забудем поменять во всех местах пользования.

    Поэтому мне тут кажется более правильным возвращать структуру, которая в себе хранит ошибку, которая может быть None. Это не обязательно может быть dataclass, можно сделать, например, tuple:

    async def invoke(
    self,
    param: CrossIdentificationParam,
    simultaneous_data_provider: CrossIdSimultaneousDataProvider,
    user_param: CrossIdentificationUserParam,
) -> tuple[result.CrossIdentifySuccess | None, CrossIdentificationError | None]:
  if param.names is None and param.coordinates is None:
      return None, CrossIdentificationEmptyError
  ....

  return res, None

    И тогда в месте использования будет что-то типа

      result, err = CrossIdentifyUseCase().invoke(.....)
  if err is not None:
    'возвращаем ошибку'

  'работаем с результатом'

    Если не нравится, что они могут оба быть None, можешь сделать результат классом, в конструкторе которого принимается два аргумента, один из которых обязан быть не None, тогда у тебя будет гарантия что никто не сможет собрать инстанс класса в котором оба параметра None.

d-makarov-d commented 2 months ago

С первой частью я и не спорил, тут мой косяк, это да, везде плохая практика

Со второй частью не совсем согласен, в плане возможных ошибок - как раз в типе указано, что может быть только CrossIdentificationException и все ее подклассы. И собственно, раницы мажду Union или tuple или dataclass с типизацией принципиальной нет.

А в целом, если тебе прям режет глаз, без б, закоммичу вариант с классом и проверкой в конструкторе. Более того, я уже зарефакторил на класс Result, но на мой взгляд, лучше не стало)

Kraysent commented 2 months ago

Со второй частью не совсем согласен, в плане возможных ошибок - как раз в типе указано, что может быть только CrossIdentificationException и все ее подклассы. И собственно, раницы мажду Union или tuple или dataclass с типизацией принципиальной нет.

А в целом, если тебе прям режет глаз, без б, закоммичу вариант с классом и проверкой в конструкторе. Более того, я уже зарефакторил на класс Result, но на мой взгляд, лучше не стало)

Ну я имею ввиду, что если поменять код в соответствии с первой частью, то получится, что ошибки начнут скрываться - возвращаемое значение останется только одно. Я собственно и предлагаю делать не одно возвращаемое значение, а два, примерно как у тебя сейчас)

Между Union и Tuple разница в том, что в union можно забыть проверить на разные типы, а с tuple забыть точно не получится, потому что возвращаемых значений всегда два

d-makarov-d commented 2 months ago

Так, ладно, по итогам дебатов. А то я уже в этих ветках потерялся.

Все так, ничего не забыл?

Kraysent commented 2 months ago

Так, ладно, по итогам дебатов. А то я уже в этих ветках потерялся.

  • по async выпиливаю здесь, где могу, остальное в issue
  • по возвращению эксепшенов - перехожу на возвращение класса из юзкейса, остальное в issue
  • делаю эксепшены более специальными, чтобы не передавать param
  • мелкие правки по стилю
  • по executemany - отдельная ишью, где повторяю ошибку и решаем, че с ней делать

Все так, ничего не забыл?

Вроде норм

d-makarov-d commented 2 months ago