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

create function "years_select" which shows data according to input year #19

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

DarthProvader
Copy link
Collaborator

No description provided.

OpposedOak
OpposedOak previously approved these changes Mar 2, 2021
Copy link
Collaborator

@OpposedOak OpposedOak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Function get_daily_average_temperatures created
get_daily_average_temperatures changed so it takes correct date format
get_yearly_average_temperatures created - returns disctionary of average temperatures yearly
@zitkat
Copy link
Collaborator

zitkat commented Mar 14, 2021

Ahoj, navrhuji, abyste si mergnuli main s tím že změny v process_meteo_data.py přijmete z main, api_meteo_data.py necháte tak jak je. To je asi nejsnažší udělat přes nějaké IDE, kdybste měli čas můžeme to rychle udělat spolu, v nejhorším to můžu udělat i já. Potřebné věci pak naimportujte z process_meteo_data.py.

@DarthProvader
Copy link
Collaborator Author

Ahoj,
Dneska v 6 máme s Vaškem v plánu na to kouknout. Pokud máš čas, tak by ses mohl přidat a uděláme to společně.

@zitkat
Copy link
Collaborator

zitkat commented Mar 14, 2021

Dneska v 6 máme s Vaškem v plánu na to kouknout. Pokud máš čas, tak by ses mohl přidat a uděláme to společně.

Ok, tak jo, tak mi pak akorát prosím napište nějakou soukromou zprávu (mail, FB, ...) jak se spojíme :-)

@DarthProvader
Copy link
Collaborator Author

Přidal jsem si tě na fb. Většinou jsme na https://meet.vpsfree.cz/pythonplzen ale to případně ještě potvrdim

@Mintaka
Copy link
Owner

Mintaka commented Mar 14, 2021

Vypadá to dobře.
Teď si jen dát tu práci to přeskládat do současného main.
Jaký @zitkat navrhuješ způsob?

@DarthProvader
Copy link
Collaborator Author

Vypadá to dobře.
Teď si jen dát tu práci to přeskládat do současného main.
Jaký @zitkat navrhuješ způsob?

Už jsme to s Tomášem vyřešili. Teď už process_meteo_data vypadá stejně jako v main a náš soubor je oddělen takže si vždycky můžeme udělat git pull a api bude pracovat s aktuální databází/programem.

OpposedOak and others added 5 commits March 15, 2021 10:27
Function "get_monthly_average " has been added
Bug Fix in for loop  "for k,v in year_month.items(): "in function "get_monthly_average_temperatures"
New approach for function  "get_weekly_average_temperatures"
Copy link
Collaborator

@zitkat zitkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, tak jsem zjistil, že pokud člověk nenapíše ještě nějaké milé úvodní slovo tak se review nezobrazuje a já se divím, že nikdo nereaguje :-(

Comment on lines 21 to 26
years = []
results = {}
count_years = date_from_y
for rok in range((date_to_y - date_from_y) + 1): #create list called "years" which consist of range of years from selected dates (eg 1995-05-4 and 1998-04-09 creates years[1995, 1996, 1997, 1998])
years.append(count_years)
count_years += 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proč generovat roky, které chceme zprůměrovat takhle složitě na vytváření posloupností máme přece v pythonu funkci ne? Tou půjde zjednodušit i kód níže ;-)
Tip: kdykoliv máte ve for cyklu jen tak volně výraz

něco += 1

je to podezdřelé.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mohl bych se zeptat jakou funkci máš na mysli? Generovat roky chci abych si udělal seznam roků a nad každým udělal query. Jak jinak by se to dalo provést?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asi jsem měl napovědět trochu jasněji, mám na mysli range, proč nepoužít range ke generování roků přímo místo všeho toho počítání?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tak snad by to takhle mělo být lepší
c644be9

Comment on lines 29 to 34
for rok in years:
query = cursor.execute(f"""SELECT Avg(temperature) FROM temperatures
WHERE date LIKE '{years[i]}%'""")
avg_temp = str(query.fetchall())
results[f"{years[i]}"] = avg_temp[2:5] #for every year get average temperature and put it into the dictionary as string
i += 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proč se v cyklu nikde nevyužívá proměnná rok? Co se do ní v průběhu cyklu ukládá? Je nutné převádět proměnnou year na řetězec pro indexování do slovníku?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opraveno v ce5f434
Nyní je type(avg_temp) = float, místo "i" jsem použil pro loop proměnnou "rok".
Omezeno na 2 desetinná místa.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float je ok, proč se ale omezovat na dvě desetinná místa? Obdobně jako výše, proč generovat roky dopředu, když si je můžu vyrobit pomocí range a ušetřit si tak dokonce dvě proměnné a indexování do seznamu years?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c644be9
viz výše + odstraněny restrikce na 2 des. čísla

Comment on lines +88 to +89
avg_temp = str(query.fetchall())
results_month [i] = avg_temp[2:7]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funkce vracející průměry by neměla vracet řetězce.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ce5f434
Měl by rok být taky integer?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asi jo, rozhodně se s nimi bude později lépe pracovat.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


year_actuall += 1

for k,v in year_month.items():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k a v nejsou vhodné názvy proměnných.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Přejmenuji. Jsou to zatím jen pracovní názvy.

months = {}
count_months = date_from_m
for month in range(date_from_m,date_to_m + 1):
months [month] = "NaN"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pokud se hranaté závorky používají k indexaci nedělá se před nimi mezera, podobně jako při volání funkce.

months = {}
count_months = date_from_m
for month in range(date_from_m,date_to_m + 1):
months [month] = "NaN"
Copy link
Collaborator

@zitkat zitkat Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do slovníku months se uloží jen samé "NaN", nikdy žádná užitečná data a požívají se jen klíče, není k tomu lepší seznam?

Copy link
Collaborator

@OpposedOak OpposedOak Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Řetežec "NaN" funguje jako place-holder, později se na jeho místo přiřadí teplota z databáze. Šlo by to udělat jednodušeji ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ať koukám jak koukám, tak nikde nevidím, že by se tam něco později zapisovalo. Na konci funkce jsou v ní pořad jen ty NaNy

count_months = date_from_m
for month in range(date_from_m,date_to_m + 1):
months [month] = "NaN"
count_months += 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K čemu je proměnná count_monthsjejí hodnota se nikde nepoužije.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proměnná vyřazena

date_to_m = int(list(date_to.split("-"))[1])

year_month = {}
results_month = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proměnná results_months se nikde nepoužije.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Používá se na přiřazování hodnot do slovníku results: "results [k] = results_month". Určitě bude existovat lepší způsob

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Už to vidím, inicializuje se při každém průchodu cyklem

for k,v in year_month.items():

takže to z pohledu řádku 45 vypadá, že se nepoužívá. Tuhle inicializaci můžete smazat, to už je ale drobnost :-)

Comment on lines +49 to +81
while year_actuall != date_to_y + 1:

if date_from_y == date_to_y:
months = {}
count_months = date_from_m
for month in range(date_from_m,date_to_m + 1):
months [month] = "NaN"
count_months += 1
year_month [year_actuall] = months

if (year_actuall == date_from_y) and date_from_y != date_to_y:
months = {}
count_months = date_from_m
for month in range(date_from_m,13):
months [month] = "NaN"
count_months += 1
year_month [year_actuall] = months

elif year_actuall == date_to_y:
months = {}
count_months = 1
for month in range(1,date_to_m + 1):
months [month] = "NaN"
count_months += 1
year_month [year_actuall] = months

else:
months = {}
for month in range(1,13):
months [month] = "NaN"
year_month [year_actuall] = months

year_actuall += 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tenhle kód je dost dlouhý, i po přípomínkách výše nejspíš bude, hodilo by se udělat z něj funkci, napadá mě i jméno, generate_years_months.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do funkce ho můžeme přesunutou, pokud budeme zachovávat způsob sestavení slovníků pro výběr teplot

@zitkat zitkat dismissed OpposedOak’s stale review March 22, 2021 07:21

Ještě to vylepšíme ;-)

DarthProvader and others added 5 commits March 22, 2021 19:24
Function explanations added, meteostation param is taken into consideration and some loops updated
get_yearly_data simplified loops and now returns dictionary as {int(year):float(avg_temp)}
No restrictions for decimals in avg temp
Data are grouped by week
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.

4 participants