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

Remove MetricResult.config. #513

Merged
merged 45 commits into from
Sep 30, 2022
Merged

Remove MetricResult.config. #513

merged 45 commits into from
Sep 30, 2022

Conversation

odashi
Copy link
Contributor

@odashi odashi commented Sep 29, 2022

Related to #491

This change removes MetricResult.config. This member is not used at this point, and we basically don't need to store the same config in this struct because we can obtain the same information directly from Metric.

tetsuok and others added 30 commits August 30, 2022 18:28
Co-authored-by: Graham Neubig <neubig@gmail.com>
@odashi odashi requested a review from neubig September 29, 2022 21:10
Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

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

Quick clarification: When writing out the results of an analysis, I believe only MetricResult is serialized, not Metric. How would it be possible to get the information about Metric when loading in the results from a serialized file?

@odashi
Copy link
Contributor Author

odashi commented Sep 30, 2022

I think MetricConfig (or Metric) should be serialized separately as well. I also think parallel structure is more maintainable than nested structure.

"metric_configs": {
  "foo": <foo_config>,
  "bar": <bar_config>
},
"metric_results": {
  "foo": <foo_result>,
  "bar": <bar_result>
}

@neubig
Copy link
Contributor

neubig commented Sep 30, 2022

OK, sounds good. I'll review this.

@odashi odashi merged commit d888d3a into main Sep 30, 2022
@odashi odashi deleted the dict-metricresult branch September 30, 2022 20:32
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