Conversation
neuralmonkey/learning_utils.py Outdated
| | ||
| supported_type = Union[ | ||
| List[Dict[str, np.ndarray]], | ||
| List[Dict[str, Union[np.ndarray, np.float32]]], |
There was a problem hiding this comment.
Je to určitě np.float32 nemá to být normální float?
There was a problem hiding this comment.
Je to kvuli gradient_runneru, ktery vraci dict gradientu a ty jsou vystupem volani session.run, takze myslim, ze np.float32 je na miste
There was a problem hiding this comment.
@varisd má pravdu. session.run() vrací numpyovský floaty, který nejsou subclassy od pythonovskejch floatů, takže by to padalo.
| image_summaries=None) | ||
| | ||
| | ||
| class GradientRunner(BaseRunner[SupportedDecoder]): |
There was a problem hiding this comment.
Dokumentace: co to dělá, k čeu je tpo potenciálně dobrý.
| gradient_dict[name] = val | ||
| | ||
| self.result = ExecutionResult( | ||
| outputs=[gradient_dict], |
There was a problem hiding this comment.
Musí to být v tom listu?
There was a problem hiding this comment.
Jo, jinak ti to bude vracet, jen nazvy tech promennych a ne dicty. Muze za to tento radek:
https://github.com/ufal/neuralmonkey/blob/master/neuralmonkey/runners/base_runner.py#L77
| decoders: List[Any], | ||
| decoder_weights: List[ObjectiveWeight] = None, | ||
| l1_weight: float = 0., | ||
| l2_weight: float = 0., |
There was a problem hiding this comment.
Já bych tady tu L2 a L1 nechal jako syntactic sugar a v konftruktoru udělal regularizátora, co zkonstruuje ty regularizátoři a přidá je to listu.
| class EWCRegularizer(BaseRegularizer): | ||
| """Regularizer based on the Elastic Weight Consolidation. | ||
| | ||
| TODO description |
| from neuralmonkey.logging import log | ||
| | ||
| | ||
| class BaseRegularizer: |
There was a problem hiding this comment.
Pojmenova bych to jenom Regularizer
There was a problem hiding this comment.
Akorát bez toho překlepu :-D
| Note, that currently this branch removes the default l1, l2 logging into summaries |
| Oh, I didn't even notice. I'd like to keep the L2 plot in TensorBoard. |
| The L2 plot is back. |
jindrahelcl left a comment
jindrahelcl left a comment There was a problem hiding this comment.
Přestože jsi změnil milion testů, funkcionalita, kterou tady introducuješ (jmenovitě gradient runner a EWC regularizer) zůstává netestovaná.
Refaktor na regularizery mi přijde užitečnej, ale ještě bych se zamyslel, jestli váha regularizeru je opravdu součástí regularizeru. Kdyby to tak nebylo, odpadly by trampoty s defaultníma vahama, kterejm se věnuju v tom review.
| regularizers = [] | ||
| if l1_weight > 0.: | ||
| if L1Regularizer in [type(r) for r in regularizers]: | ||
| warn("You specified both trainer l1_weight " |
There was a problem hiding this comment.
já bych tady newarnoval, ale rovnou chcípal.
There was a problem hiding this comment.
... a jak na to koukám, tak je to docela humus, protože na ten seznam regularizes neimposuješ žádný constrainty, takže tam klidně dva L1 regularizery bejt můžou a stěžovat si to nebude.
... a co když někdo podědí od L1 regularizeru (třeba aby mu dal předdefiovanou váhu)? to to projde bez varování, protože type(r) nebude L1Regularizer.
There was a problem hiding this comment.
proto tam je warn (kdyby si nahodou nevsim, ze to definuje dvema zpusoby)... nechci uzivateli svazovat ruce, at kldine do seznamu hodi L1Regularizeru, kolik chce
There was a problem hiding this comment.
házet víc stejnějch regularizerů je blbost a mělo by to spadnout protože je to téměř jistě něco, co nechceš. Stačí sečíst ty weights a bude ti stačit jen jeden.
There was a problem hiding this comment.
klidne muzu to kontrolu vyhodit uplne, co ja vim
| | ||
| if l2_weight > 0.: | ||
| if L2Regularizer in [type(r) for r in regularizers]: | ||
| warn("You specified both trainer l2_weight " |
| from neuralmonkey.runners.base_runner import ( | ||
| Executable, ExecutionResult, NextExecute) | ||
| from neuralmonkey.trainers.regularizers import ( | ||
| Regularizer, L2Regularizer) |
There was a problem hiding this comment.
to se nevejde na jednu řádku?
| collections=["summary_train"]) | ||
| self.losses = [o.loss for o in objectives] + reg_values | ||
| | ||
| # we always want to include l2 values in the summary |
There was a problem hiding this comment.
nechápu. ty grafy nejsou stejný a každá diagnostická informace dobrá, nebo ne?
There was a problem hiding this comment.
myslim, ze docela koreluji + tys je nekdy pouzival? (vetsinou ti staci l2)
There was a problem hiding this comment.
to že dvě veličiny korelujou neznamená, že je zajímavá jen jedna. už jsme viděli i grafy kde L1 rostla a L2 klesala.
There was a problem hiding this comment.
hod mi use case, kdy ti v minulosti analyza L1 pomohla
There was a problem hiding this comment.
hoď mi důkaz, že mi v budoucnosti nemůže pomoct
| | ||
| # we always want to include l2 values in the summary | ||
| if L2Regularizer not in [type(r) for r in self.regularizers]: | ||
| reg_values.append(L2Regularizer().value(regularizable)) |
There was a problem hiding this comment.
nešlo by to udělat nějakejma statickejma metodama? tady takhle konstruovat něco, co se konstruuje z konfigu mi přijde ohavný
There was a problem hiding this comment.
navíc, co je tohle vůbec za humus? :-) Přidáváš tady do reg_values, který pak zipuješ s něčím, kam jsi nic nepřidal, takže se to tam afaik stejně neukáže. reg_values se nesmí měnit (taky kvůli sémantice - obsahuje to hodnoty regularizátorů, ale já L2 nepoužívám jako regularizátor, takže by jeho hodnota neměla bejt v tomhle seznamu)
udělal bych tady zvláštní if a v něm rovnou volal tf.summary.scalar
There was a problem hiding this comment.
Ten L2Regularizer by se mel pridat i do self.regularizers.
reg_values se potom, co se pridaji do summaries, dale uz nikde nepouzivaji, takze neni problem tam pridat tenhle "default" regularizer
There was a problem hiding this comment.
Jenže to neni regularizer. Takže až k tomu za měsíc někdo přijde a bude chtít něco dělat s regularizerama, tak se tenhle sémantickej bug projeví.
Jde mi jen o to, aby v proměnnejch, co se nějak jmenujou, byly věci, který tomu odpovídaj.
A neadresoval jsi mojí poznámku o tom, že by se neměl volat konstruktor, ale že by to mělo bejt buď funkcí nebo statickou metodou.
| | ||
| def __init__(self, | ||
| name: str = "train_l1", | ||
| weight: float = 1.0e-8) -> None: |
There was a problem hiding this comment.
už jsem to napsal níž - vyházet neopodstatněný defaultní hodnoty u name i u weight
pokud se nějaká hodnota doporučuje (což u L1 ani L2 neni ten případ), napsal bych to do docstringu.
| | ||
| log("Loading gradient estimates from {}".format(gradients_file)) | ||
| self.gradients = np.load(gradients_file) | ||
| log("Gradient estimates loaded") |
There was a problem hiding this comment.
tečky za větama v lozích (4x) :-)
There was a problem hiding this comment.
Ok, ale tohle neplati pro celou codebase
There was a problem hiding this comment.
No jo no, ale když to vidim tak vidim že by tam asi měla bejt...
| self.gradients = np.load(gradients_file) | ||
| log("Gradient estimates loaded") | ||
| | ||
| def value(self, variables: List[tf.Tensor]) -> float: |
There was a problem hiding this comment.
tahle funkce chce trochu komentářů nebo docstring co se tu děje
| if (var_name in self.gradients.files | ||
| and self.init_vars.has_tensor(var_name)): | ||
| init_var = self.init_vars.get_tensor(var_name) | ||
| gradient = tf.constant( |
There was a problem hiding this comment.
proč to tady obaluješ tou konstantou? tf.square neakceptuje čísla? a co víc, nemůžeš si tu druhou mocninu předpočítat mimo tensorflow?
There was a problem hiding this comment.
ad constanta: chci ty gradienty mit v grafu pojmenovane
ad predpocitani: muzu, dobry napad
There was a problem hiding this comment.
jestli to je kvůli jménu, tak je to asi takhle dobrý, protože kdyby sis předpočítával tu druhou mocninu vedle, tak bys pak ten gradient v grafu vůbec neměl.
There was a problem hiding this comment.
momentalne to resim... je dobre to mit pojmenovane, square se predpocita o krok drive v numpy... jeste jsem to nepushnul
| return ewc_value | ||
| | ||
| | ||
| L1 = L1Regularizer() |
There was a problem hiding this comment.
tady bych tu defalutní váhu asi dal, ale napsal bych to do modulovýho dokstringu. Ale i tak mam strach že dostupnost defaultní hodnoty pro tyhle regularizery dá userům pocit, že defaultní hodnota je to, co maj použít, ale to vůbec neni pravda, tadyta 1e-8 je založená na nějaký jiný defaultní hodnotě, pro kterou nemáme žádný vysvětlení.
There was a problem hiding this comment.
Tak tyhlety zkratky vyhodim
| Právě kvůli ty sémantice bych to nedělal.. regularizers jsou regularizers a new regularizers and then some.. Dne pá 3. 8. 2018 21:48 uživatel Dušan Variš <notifications@github.com> napsal: … ***@***.**** commented on this pull request. ------------------------------ In neuralmonkey/trainers/generic_trainer.py <#742 (comment)>: > # unweighted losses for fetching - self.losses = [o.loss for o in objectives] + [l1_value, l2_value] - tf.summary.scalar("train_l1", l1_value, - collections=["summary_train"]) - tf.summary.scalar("train_l2", l2_value, - collections=["summary_train"]) + self.losses = [o.loss for o in objectives] + reg_values + + # we always want to include l2 values in the summary + if L2Regularizer not in [type(r) for r in self.regularizers]: + reg_values.append(L2Regularizer().value(regularizable)) Ten L2Regularizer by se mel pridat i do self.regularizers. reg_values se potom, co se pridaji do summaries, dale uz nikde nepouzivaji, takze neni problem tam pridat tenhle "default" regularizer — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwcs4sS68v6cTEfJRcyTu-TgJa6xpDFks5uNKkLgaJpZM4VkjNe> . |
| S/new/ne/ Dne pá 3. 8. 2018 21:54 uživatel Jindra Helcl <jindra.helcl@gmail.com> napsal: … Právě kvůli ty sémantice bych to nedělal.. regularizers jsou regularizers a new regularizers and then some.. Dne pá 3. 8. 2018 21:48 uživatel Dušan Variš ***@***.***> napsal: > ***@***.**** commented on this pull request. > ------------------------------ > > In neuralmonkey/trainers/generic_trainer.py > <#742 (comment)>: > > > > # unweighted losses for fetching > - self.losses = [o.loss for o in objectives] + [l1_value, l2_value] > - tf.summary.scalar("train_l1", l1_value, > - collections=["summary_train"]) > - tf.summary.scalar("train_l2", l2_value, > - collections=["summary_train"]) > + self.losses = [o.loss for o in objectives] + reg_values > + > + # we always want to include l2 values in the summary > + if L2Regularizer not in [type(r) for r in self.regularizers]: > + reg_values.append(L2Regularizer().value(regularizable)) > > Ten L2Regularizer by se mel pridat i do self.regularizers. > > reg_values se potom, co se pridaji do summaries, dale uz nikde > nepouzivaji, takze neni problem tam pridat tenhle "default" regularizer > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#742 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABwcs4sS68v6cTEfJRcyTu-TgJa6xpDFks5uNKkLgaJpZM4VkjNe> > . > |
| Executable, ExecutionResult, NextExecute) | ||
| from neuralmonkey.trainers.regularizers import ( | ||
| Regularizer, L2Regularizer) | ||
| from neuralmonkey.trainers.regularizers import (Regularizer, L2Regularizer) |
| | ||
| | ||
| # pylint: disable=too-few-public-methods,too-many-locals,too-many-branches | ||
| # pylint: disable=too-many-statements |
There was a problem hiding this comment.
too many statements znamená, že by se to mělo rozsekat na funkce. nejde to nějak?
There was a problem hiding this comment.
souhlasim, nechtelo se mi v tom vcera vrtat... kazdopadne to jeste v tomhle PR zkusim zapracovat
| collections=["summary_train"]) | ||
| self.losses = [o.loss for o in objectives] + reg_values | ||
| | ||
| # we always want to include l2 values in the summary |
There was a problem hiding this comment.
hoď mi důkaz, že mi v budoucnosti nemůže pomoct
| class Regularizer: | ||
| """Base class for the regularizers.""" | ||
| class Regularizer(metaclass=ABCMeta): | ||
| """Base clas s for regularizers. |
| """Base clas s for regularizers. | ||
| | ||
| Regularizer objects are used to introduce additional loss terms to | ||
| the trainerthus constraining the model variable during training. These |
There was a problem hiding this comment.
s/trainerthus/trainer, thus/
| | ||
| Implements Elastic Weight Consolidation from the "Overcoming catastrophic | ||
| forgetting in neural networks" paper. | ||
| The regularizer applies separate regularization weight to each trainable |
| Implements Elastic Weight Consolidation from the "Overcoming catastrophic | ||
| forgetting in neural networks" paper. | ||
| The regularizer applies separate regularization weight to each trainable | ||
| variable based on how important the variable was for the previously |
There was a problem hiding this comment.
based on its importance for the previously learned task
| | ||
| log("Loading initial variables for EWC from {}".format(variables_file)) | ||
| log("Loading initial variables for EWC from " | ||
| "{}.".format(variables_file)) |
There was a problem hiding this comment.
konkatenaci stringů nedělej a druhej řádek (pokud se to fakt nevejde na jednu) začni .format.
| ewc_value = tf.constant(0.0) | ||
| for var in variables: | ||
| var_name = var.name.split(":")[0] | ||
| var_name = var.name |
There was a problem hiding this comment.
uvědomuješ si, že přístup přes tečku nic nestojí a že tohle je vlastně kopírování jedný lokální proměnný do druhý, jejíž jméno se liší jedním znakem?
| name="{}_init_value".format(init_var_name)) | ||
| grad_squared = tf.constant( | ||
| np.square(self.gradients[var_name]), | ||
| name="{}_ewc_weight".format(init_var_name)) |
There was a problem hiding this comment.
Teď si ale právě nepojmenováváš v grafu gradienty, ale jejich druhý mocniny. To jsi mohl docílit už tím, že bys napsal tf.square(gradient, name="kráva")
There was a problem hiding this comment.
nevim, takhle ale uz do grafu pridavam predpociane druhe mocniny... v opacnem pripade (s tf.square) by se ten vypocet provadel pokazde, kdyz bys potreboval ten vysledek
ve vysledku tam asi takovy rozdil v rychlosti nebude, ale prijde mi to rozumnejsi si konstanty predpocitat, nez je fouknes do grafu
jindrahelcl left a comment
jindrahelcl left a comment There was a problem hiding this comment.
Chápu správně, že GradientRunner se používá s natrénovaným modelem, na kterej na jednu nějakou batch aplikuju trainer, kterej vrátí nějaký gradienty?
Btw. pořád to neni testovaný v žádným testu. Navrhuju, aby jeden test seběhnul ten runner, kterej to někde uloží, a druhej test pustil ten EWC regularizer. Anebo to jen fikaně přidat do testu, kterej má ukládání už pořešený. Zkrátka aby se použil jak ewc regularizer, tak gradient runner.
Jinak se mi líbí jak už to hezky konverguje k mergovatelnosti. :-)
tests/nematus.ini Outdated
| class=trainers.cross_entropy_trainer.CrossEntropyTrainer | ||
| decoders=[<decoder>] | ||
| l2_weight=1.0e-8 | ||
| regularizers=[trainers.regularizers.L2] |
There was a problem hiding this comment.
klidně bych to z těch testů vyhodil a v jednom jich otestoval víc
| Jo, testy jsem jeste neresil. Ten gradient runner pouzivam, abych si vyprintil gradienty z celeho datasetu (vetsinou validacniho, protoze neni tak velky). Z toho si pak bokem udelam prumer gradientu pro kazdou vahu modelu; ty pak muzu pouzit v EWC. |
| Tak kdyby se ti podařilo tuhle pipelinu nasimulovat v |
| ping, uz jsem tuhle vetev rebasoval minimalne trikrat... pokud ji nemate v planu mergovat, dejte mi vedet |
| Mno já jsem myslel, že se to zamerguje, až bude hotovej refaktoříček s tf datasetem (stejně jako hromada commitů, který si syslim ve štelářku a nedělám z toho PR). |
3 participants
NOTE: currently is not working with DelayedUpdateTrainer due to a problem with trainer variable restoration.