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

Hparams: Set interval domain fields for float summary hparams. #6574

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

yatbear
Copy link
Member

@yatbear yatbear commented Sep 12, 2023

Currently TF summary written hparams with DATA_TYPE_FLOAT64 value type have no domain types.

#hparams

Copy link
Contributor

@rileyajones rileyajones left a comment

Choose a reason for hiding this comment

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

Thank you for quickly diving in and fixing this!

if result.type == api_pb2.DATA_TYPE_FLOAT64:
# Always uses interval domain type for numeric hparam values.
distinct_float_values = sorted(
[v.number_value for v in values if v.HasField("number_value")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the if v.HasField("number_value") necessary? Seems like the logic L312 to L322 assures that if result.type == api_pb2.DATA_TYPE_FLOAT64 then all the values have a number_value field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! Removed the field check.

@yatbear yatbear merged commit 1dde0cb into tensorflow:master Sep 14, 2023
13 checks passed
@yatbear yatbear deleted the domain branch September 14, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants