Observational data on old plots by GeorgySk · Pull Request #29 · GeorgySk/alcor

GeorgySk · 2017-09-22T21:33:07Z

Now we also plot observational data on old plots of velocities vs magnitude and velocities clouds diagrams

lycantropos · 2017-09-23T02:03:02Z

alcor/services/plots/velocities_vs_magnitude.py

+    observational_clouds_paths = dict(
+            u=os.path.join(base_dir, u_observational_clouds_path),
+            v=os.path.join(base_dir, v_observational_clouds_path),
+            w=os.path.join(base_dir, w_observational_clouds_path))


но ведь эти вещи можно передавать сверху, что облегчит сигнатуру и уберёт значения по умолчанию, которые не стоит использовать для путей, потому что у всех они разные, а при изменении никто не обратит внимание на функцию на самом дне

сверху - это откуда именно? из функции draw модуля plots.service в месте вызова?

ага, в месте вызова, этой функции будет без разницы, откуда именно её вызывают, лишь бы передали словарь с путями

lycantropos · 2017-09-23T02:04:28Z

alcor/services/plots/velocities_vs_magnitude.py

+    v_observational_bins_df = pd.read_csv(observational_bins_paths['v'],
+                                          delimiter=' ',
+                                          skipinitialspace=True,
+                                          header=None)


DRY, что нам мешает использовать partial?

запилил

lycantropos · 2017-09-23T02:09:22Z

alcor/services/plots/velocities_vs_magnitude.py

+    w_observational_clouds_df = pd.read_csv(observational_clouds_paths['w'],
+                                            delimiter=' ',
+                                            skipinitialspace=True,
+                                            header=None)


6 раз (!)

вместо того, чтобы делать простой Copy-Paste внутри проекта без изменения логики с инициализацией объекта (что уже само говорит о нарушении DRY-принципа) ничто нам не стоит потратить некоторое время на вынос в отдельную функцию (если это всё ещё не функция) или на фиксирование значений у уже имеющейся, если часть не изменяется

и мы в очередной раз пройдём путь, когда все эти префиксы u, v, w – суть проекции данных, логика которых изменяется только на префикс

в этом прелесть физики, в наличии всевозможных инвариантов, и мы должны использовать эту фичу, а не копипастить

lycantropos · 2017-09-23T02:19:10Z

alcor/services/plots/velocities_vs_magnitude.py

+                 y_line_obs=v_observational_bins_df[1],
+                 yerr_obs=v_observational_bins_df[2],
+                 x_scatter_obs=v_observational_clouds_df[0],
+                 y_scatter_obs=v_observational_clouds_df[1])


эти вызовы отличаются лишь префиксом u/v/w, если исходную функцию обобщить и все эти *_vs_mag_bins. df, SAMPLE TEXT считывать сверху, а сюда просто их передавать по одному, мы уменьшим объём как минимум втрое

и это нужно сделать, потому что дальше кода будет ещё больше, нас всего 2ое, не хочется потом тратить пару недель на простой рефакторинг, который легче всего сделать, пока мы (ну я не в счёт) в контексте задачи, которую этот код должен выполнять

этот подход с написанием общей функции и передачей в неё проекций значительно упростит следующее:

чтение кода, ведь его объём уменьшится, а всё самое интересное (I/O операции) будет происходить наверху, а не на самом дне, не забываем, что современные программисты несмотря на заблуждения масс бОльшую часть времени

тестирование, которое было бы неплохо начать писать уже сейчас, ведь это увеличивает количество участников до 3, притом он всегда онлайн, отвечает в считанные секунды, говорит фактами, и чем проще – тем он лучше, потому что мы можем себе позволить пропустить незначительный edge-case'ы, но наши use-case'ы он должен покрывать, ТАКЖЕ написание unit-тестов облегчится за счёт того, что всё I/O в таком подходе уползает наружу, а не происходит в самом низу, что само по себе есть причина многих бед, надо по возможности оперировать с чистыми функциями, может показаться, что я этого начитался/насмотрелся и теперь бездумно это проповедую, но так оно и есть + я немного писал тестов и это становится тем очевиднее, чем больше их напишешь, поэтому я так против мутирования Star объектов.

Для наглядности рассмотрим пример:
есть функция, которая мутирует state одного Star-объекта:

def set_coordinates(star: Star, coordinates: Tuple[int, int, int]) -> None: # `if` здесь чтобы добавить логики if not star.coordinates: star.coordinates = coordinates

код максимально краток, при этом аналогичный тест

...generating star object and new coordinates... old_coordinates = star.coordinates set_coordinates(star, new_coordinates) assert (not old_coordinates and star.coordinates == new_coordinates or star.coordinates == old_coordinates)

не отличается ясностью. Мы оперируем не в терминах звёзд, а в терминах их внутренностей, и из этого теста можно подумать, что Star – абстракция, которая «заточила» в себе бедные объекты и нам приходится их освобождать (создавать old_coordinates), чтобы работать наконец с самими объектами, этот подход противоречит самой сути абстракции и ООП, когда ты должен скрывать реализацию и просто обмениваться сообщениями а-ля

...generating star object and new coordinates... new_star = update_coordinates(star, new_coordinates) assert (not star.coordinates and new_star.coordinates == new_coordinates or new_star.coordinates == star.coordinates)

т. е. в данном тесте coordinates уже спокойно может быть как полем, так и property (напомню, что основная суть property в том, что мы предоставляем динамический getter (и опционально setter/deleter) для объектов, и в отсутствие setter/deleter, пользователь не может напрямую изменить поле объекта, т. е. оно становится read-only, а если ко всем полям относиться read-only, то возникает очевидная возможность мемоизации (с которой нужно быть осторожным, но «we are adults here», советую это выступление посмотреть с самого начала), мы абстрагируемся от реализации Star.coordinates и работаем с ним путём общения с объектом вроде

"эй, star, что ты мне скажешь насчёт coordinates?"

а не как с продуктом в корзине, который нужно достать и посмотреть самому и так каждый раз... СНОВА И СНОВА

в рассмотренном примере первый тест будет некорректным, если например

coordinates принадлежит мутируемому типу (например list или numpy.array)

внутри set_coordinates произойдёт мутация coordinates в духе C-style:

for coordinate in new_coordinates: star.coordinates.append(coordinate)

тогда old_coordinates уже будет удовлетворять не первому условию assert'a (coordinates были пусты и перестали быть пустыми, наполнившись новыми координатами)

not old_coordinates and star.coordinates == new_coordinates

а второму (координаты были не пусты и не наполнились)

star.coordinates == old_coordinates

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

lycantropos · 2017-09-24T12:24:19Z

alcor/services/plots/velocities_vs_magnitude.py

+                      w=[star.w_velocity
+                         for star in velocity_vs_magnitude_clouds['w']])
+
+    for key in ('u', 'v', 'w'):


разве я не писал про ту утилиту для прохождения по одинаковым ключам словарей?

тем более что опять нужно всё это превратить в функцию для одной проекции и работать с ней, а сверху просто передавать их одну за другой, так будет «чище»

хотел было глянуть, но подумал, что раз я решил избавиться от пост-процессинга, то сначала и избавлюсь от него. иначе, возможно, сейчас бы зря переписывал этот модуль.

lycantropos · 2017-09-24T12:25:50Z

alcor/services/plots/velocities_vs_magnitude.py

+         avg_velocities[key],
+         velocities_std[key]) = zip(*sorted(zip(bins_avg_magnitudes[key],
+                                                avg_velocities[key],
+                                                velocities_std[key])))


это ещё зачем?

если нужна сортировка, её стоит делать отдельно от draw_subplot

остаток от кассандры. она перемешивала всё. графики непоследовательно строились. надо будет ветку с фиксом сделать, где удалю эти сортировки

… be implemented

…tude

GeorgySk · 2017-10-09T10:52:31Z

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

GeorgySk requested a review from lycantropos September 22, 2017 21:47

lycantropos reviewed Sep 23, 2017

View reviewed changes

lycantropos reviewed Sep 24, 2017

View reviewed changes

GeorgySk force-pushed the sloan_cones branch from c8ac9e6 to 88d3b63 Compare September 28, 2017 17:17

GeorgySk changed the base branch from sloan_cones to master September 29, 2017 19:55

GeorgySk added 16 commits September 29, 2017 22:07

Fortran: renamed thick disk params in cone.f

b7e993a

Fortran: fixed bug with thick disk SFR variables

3fa310a

Fortran: implemented thick disk parameters coming from Python

6ecd361

Fortran: deleted redundant l and b ranges from function interfaces

819245b

Fortran: deleted unused l_max and b_max

8f004ee

Fortran: fixed wrong SFR time references for disks in cone.f

d7db678

Fortran: fixed names for thick disk parameters

23aa31c

Fortran: added a loop for halo WDs; normalization cone calculation to…

ef67239

… be implemented

Fixed names of star model parameters

66d43db

Fixed old parameters names of star model

8242a4e

Added observational data plotting

40a1e7e

Updated requirements

e5b24f7

Refactoring velocities_vs_magnitudes: plot_lepine_case

1dc87f6

Fixed order of UBVRI colors

479b53a

Fortran: deleted extinction calculation - it will go to plotting

45fc361

Fortran: changed total proper motion to the one by longitude and lati…

6158b73

…tude

GeorgySk force-pushed the obs_data_plots branch from 77a7649 to 38620fc Compare September 29, 2017 21:04

Fixed wrong references to to_cartesian_from_equatorial function

38620fc

GeorgySk added the enhancement label Oct 30, 2017

GeorgySk self-assigned this Oct 30, 2017

Conversation

GeorgySk commented Sep 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lycantropos Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeorgySk commented Oct 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lycantropos Sep 24, 2017 •

edited

Loading