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

Enable and refactor image summaries #162

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Conversation

cifkao
Copy link
Member

@cifkao cifkao commented Nov 30, 2016

Resolves #108.

@cifkao cifkao changed the title Enable and refactor image summaries (resolves #108) Enable and refactor image summaries Nov 30, 2016
@jindrahelcl jindrahelcl merged commit 864d8b3 into ufal:master Nov 30, 2016
@cifkao cifkao deleted the image-summaries branch November 30, 2016 19:56
@jlibovicky
Copy link
Contributor

Kucí, co jste to sem namergovali! Je to merge přes třicet commitů, to chtělo zrebasovat předtím, aby to bylo přehlednější.

A hlavě, ty attentiony chceme kreslit z validačních dat, nechceme my? (Trénovací batch je pokaždé jiná, nešlo by sledovat vývoj.) Tím pádem ty image summaries namjí být v traineru, ale v runneru. (Kód je sám o sobě dobře, ale mý být jinde.) A i kdyby jsme to chtěli kreslit na trénovacích dataech, tak na ně stejně kromě traineru pouští i runner.

Protože mi nešlo ten PR nějak hezky revertnout (asi protože je z forku a ne z naší větve?), tak jsem posunul HEAD v masteru o commit zpátky, tak doufám, takže je potřeba, Ondro, aby sis obnovil tu větev, ve které jsi to dělal.

A pro další PR, nebylo by lepší, kdyby se to dělalo rovnou ve větvi v tomhle repozitáři místo v Ondrově forku, abysme do toho mohli případně hrabat?

@cifkao cifkao restored the image-summaries branch November 30, 2016 21:33
@jindrahelcl
Copy link
Member

Omg omg já se kouknul na Files changed a tam žádný výrazný změny nebyly..

@martinpopel
Copy link
Member

Souhlasím, že když má @cifkao push access do tohoto repo, tak mohl tu větev vytvořit zde a ne ve svém forku.
I když ji vytvoří ve svém forku (kam nemáte push access), tak mu můžete poslat pull request do té jeho větve, čili PR do PR (já vím, zní to trochu sprostě).
Pokud ale chcete, aby se třicet commitů squashlo do menšího množství smysluplně ucelených commitů, tak to se musí jinak (já tedy v tomto PR vidím jen jeden commit a ne třicet).
V některých projektech jsem viděl, že místo force push do větve původního PR preferují založení nového PR, který je rebasnutý na aktuální master a commity z původního PR jsou tak squashnuté, jak je potřeba. V prvním komentáři je pak odkaz na původní PR (klidně i vícekrát např. PR 900 je rebase 778, což je rebase 700 atd.)

@jindrahelcl
Copy link
Member

Já už jsem Ondru přidal do UFAL skupiny.

@jlibovicky
Copy link
Contributor

@martinpopel: Tenhle PR má jeden commit, ale třicet jich zatím bylo v masteru od forku. To zbytečně znepřehledňuje graf commitů, když to bez problémů šlo rebasovat a mergovat to jenom přes jeden commit.

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