Skip to content

Conversation

Arquestro
Copy link
Contributor

Description

All metrics now will be collected in list, and written with best, n_best, last checkpoint metrics.

Related Issue

#431

Type of Change

  • Examples / docs / tutorials / contributors update
  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the CODE_OF_CONDUCT document.
  • I have read the CONTRIBUTING document.
  • I have checked the code-style using make check-style.
  • I have written the docstring in Google format for all the methods and classes that I used.
  • I have checked the docs using make check-docs.

All metrics now will be collected in list, and written with best, n_best, last checkpoint metrics.
@Arquestro
Copy link
Contributor Author

I know, i haven't made any changes to BaseCheckpointCallback here. But i wonder why do we have to do it this way, changing base class, if such logic is complex enough?

@Arquestro
Copy link
Contributor Author

@TezRomacH What do you think about this solution?

@TezRomacH
Copy link
Contributor

TezRomacH commented Oct 17, 2019

I'm gonna check it tonight, thanks a lot for participating ❤️

@TezRomacH
Copy link
Contributor

@Arquestro could you add the same logic to IterationCheckpointCallback?

@Arquestro
Copy link
Contributor Author

Arquestro commented Oct 18, 2019

@Arquestro could you add the same logic to IterationCheckpointCallback?

No problem for me, but are you sure that's okay to duplicate such logic? I mean, that it maybe doesn't fit the base class but double the code.

@Scitator
Copy link
Member

Scitator commented Oct 20, 2019

@Arquestro For now we can try the proposal with "double code" and then check out how we can improve it. Before that – it can be overoptimization.
Maybe, we will see the general part between them and implement good solution.

@Arquestro
Copy link
Contributor Author

Done

@Scitator Scitator merged commit 6768167 into catalyst-team:master Oct 22, 2019
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.

3 participants