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

Evaluator refactor #774

Merged
merged 7 commits into from
Nov 20, 2018
Merged

Evaluator refactor #774

merged 7 commits into from
Nov 20, 2018

Conversation

jindrahelcl
Copy link
Member

closes #667

evaluators now have a common predecessor Evaluator. Large portion of evaluator code has been simplified.

Copy link
Member

@varisd varisd left a comment

Choose a reason for hiding this comment

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

Jen nejake doplnujici otazky.
Na SacreBLEU evaluator jsem se nedival, tak snad je v pohode.

Pekny refaktor, moje OCDcko je hned stastnejsi :)

def compare_maximize(score1: float, score2: float) -> int:
# the bigger the better
return (score1 > score2) - (score1 < score2)

Copy link
Member

Choose a reason for hiding this comment

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

Musi tohle byt separatni funkce?

Copy link
Member Author

Choose a reason for hiding this comment

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

Já to dal ven, protože si lint stěžoval že no-self-use a taky jsem chtěl, aby z názvu tý funkce bylo patrný, co to dělá a aby když to chceš overridnout, tak abys mohl napsat jen compare_minimize. ale taky to jde jednodušejc, třeba že overridovaný budou dělat 1 - Evaluator.compare_scores(s1, s2) nebo prohodí s1 a s2. (myslim že takhle to někde je)


def check_lengths(scorer):
@wraps(scorer)
def decorate(self, hypotheses, references):
Copy link
Member

Choose a reason for hiding this comment

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

U tech wrapperu to nenadava na notaci?

Copy link
Member Author

Choose a reason for hiding this comment

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

když jí tam nenapíšeš, tak to nenadává. Já jí tam nechtěl psát, protože vlastně nevim, co to dělá. ale zkusim

Copy link
Member Author

Choose a reason for hiding this comment

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

nevim, co to dělá = nevim, jak se to má anotovat, když to po dekoraci může najednou vracet něco jinýho

Copy link
Member Author

Choose a reason for hiding this comment

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

mypy to ještě neumí.. z tohohle plyne, že by to jít mělo: python/mypy#3157
nicméně to podle mě pak ještě posere ten dodatečnej @wraps...

Copy link
Member

Choose a reason for hiding this comment

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

V pohode.

for m in range(1, self.n + 1):
count_all = 0
count_matched = 0
for ngr in ref_ngrams[m - 1]:
Copy link
Member

Choose a reason for hiding this comment

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

Jsem rad, ze moje politicky nekorektni zkracenina ngramu se zachovala :)

Copy link
Member Author

Choose a reason for hiding this comment

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

:-D

Copy link
Member

@varisd varisd left a comment

Choose a reason for hiding this comment

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

Za me dobry

@jindrahelcl jindrahelcl merged commit a91035b into master Nov 20, 2018
@jindrahelcl jindrahelcl deleted the evaluators branch November 20, 2018 16:58
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.

Abstract predecessor class for neuralmonkey.evaluators
2 participants