Integration/comet

[jynx10]

Before submitting (checklist)

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contribution guide?
• Did you check the code style? catalyst-make-codestyle && catalyst-check-codestyle (pip install -U catalyst-codestyle).
• Did you make sure to update the docs? We use Google format for all the methods and classes.
• Did you check the docs with make check-docs?
• Did you write any new necessary tests?
• Did you check that your code passes the unit tests pytest . ?
• Did you add your new functionality to the docs?
• Did you update the CHANGELOG?
• Did you run colab minimal CI/CD with latest and minimal requirements?

Description

Related Issue

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)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

FAQ

Please review the FAQ before submitting an issue:

• I have read the documentation and FAQ
• I have reviewed the minimal examples section
• I have checked the changelog for main framework updates
• I have read the contribution guide
• I have joined Catalyst slack (#__questions channel) for issue discussion

[DN6]

I would change the default here to 1.

Suggested change

	logging_frequency: int = 10,
	logging_frequency: int = 1,

[jynx10]

Me and Doug thought it would be better if the default value would be a value that doesn't cause the throttle.

[DN6]

I would make this f"{stage_key}/{loader_key}_{scope}" since the loader_key tells you which dataset (train, valid or test) the metric was computed on. You can have cases where the stage_key is train but the loader_key is valid. With the current implementation, these metrics will get overwritten.

Suggested change

	prefix=f"{stage_key}/{scope}",
	prefix=f"{stage_key}/{loader_key}_{scope}",

[jynx10]

In all my tests it seemed like the stage_key and the loader_key were all set on "train" so it gave the metric a weird name. You can see an example in this ticket: https://comet-ml.atlassian.net/browse/CM-781

[DN6]

So I think it's okay if the name looks weird like train/train_batch_precision_09 It would be up to the user to change the stage_key to something that is more relevant.

If the loader_key is not included, then metrics computed on the valid data would be written to the same metric name as the train data and the user would not be able to differentiate between the two.