(WIP) Add support for logging hyperparameters #87
Conversation
- added TBHparams wrapper type - added first attempt at hparams_summary - updated imports in tensorboard/tensorboard.jl to include hparams plugin
|
Remaining work:
|
|
Yes, that seems about right. If you could also write some documentation with an example for the |
|
BTW, you have a failing test case for Julia 1.0 because of an |
| EXPERIMENT_TAG = "_hparams_/experiment" | ||
| SESSION_START_INFO_TAG = "_hparams_/session_start_info" |
There was a problem hiding this comment.
You got those from tensorboardx.py?
| EXPERIMENT_TAG = "_hparams_/experiment" | ||
| SESSION_START_INFO_TAG = "_hparams_/session_start_info" | ||
|
|
||
|
|
There was a problem hiding this comment.
Could you add a few comments on what are Domain types used for? I guess it's to encode the domain of a hyper parameter but still... it would make the code more clear for future maintenance
|
|
||
| DiscreteDomainElem = Union{String, Float64, Bool} | ||
|
|
||
| hparams_datatype_sym(::Type{String}) = :DATA_TYPE_STRING |
There was a problem hiding this comment.
Sometimes SubString might be passed in. I'd support at least that one too.
Ideally, I'd say that this should be ::Type{<:AbstractString} and a conversion for exotic types should be made where appropriate
| ProtoBuf.google.protobuf.Value(x::Number) = Value(number_value=x) | ||
| ProtoBuf.google.protobuf.Value(x::Bool) = Value(bool_value=x) | ||
| ProtoBuf.google.protobuf.Value(x::AbstractString) = Value(string_value=x) | ||
| function ProtoBuf.google.protobuf.Value(x) | ||
| @warn "Cannot create a ProtoBuf.google.protobuf.Value of type $(typeof(x)), defaulting to null." | ||
| Value(null_value=Int32(0)) | ||
| end |
There was a problem hiding this comment.
This is all type-piracy, and it might stop working in the future when something is changed in ProtoBuf.jl.
Is there a specific reason you need this, and can not create your own Value function that you use locally?
You should not define specialised methods for types we do not own on functions we do not own.
Maybe, do not import Protobuf.google.protobuf.Value, define your own Value functions forwarding to Protobuf.google.protobuf.Value.
| end | ||
|
|
||
|
|
||
| function ProtoBuf.google.protobuf.ListValue(domain::DiscreteDomain{T}) where T <: DiscreteDomainElem |
There was a problem hiding this comment.
As above, type piracy. Don't import ListValue and create your own function
| struct HParam | ||
| name::AbstractString | ||
| domain::HParamDomain | ||
| display_name::AbstractString | ||
| description::AbstractString | ||
| end |
There was a problem hiding this comment.
Since we can only serialise standard tensor board types (Float32 and C-strings) I think that this here should be a type-stable struct with String types instead of AbstractStrings and proper conversion should be done in the constructor.
There was a problem hiding this comment.
Actually, it depends: is this an internal type used for logging, or the user-accessible API-type?
| description::AbstractString | ||
| dataset_type::Symbol | ||
|
|
||
| function Metric(tag::AbstractString, |
There was a problem hiding this comment.
Is the inner constructor really necessary?
| hparams::AbstractVector{HParam} | ||
| metrics::AbstractVector{Metric} |
There was a problem hiding this comment.
| hparams::AbstractVector{HParam} | |
| metrics::AbstractVector{Metric} | |
| hparams::Vector{HParam} | |
| metrics::Vector{Metric} |
| # TODO: The types in the hparam domain and this dict's values are constrained. | ||
| # e.g. an hparam with a discrete domain of ["a", "b"] must have string values | ||
| # Consider ways to enforce this relationship in the type system. | ||
| data::Dict{HParam, Any} |
There was a problem hiding this comment.
Can you explain this better? I don't understand the todo
| # FIXME: group_name auto generated in the Python implementation (Tensorboard) | ||
| group_name::AbstractString |
There was a problem hiding this comment.
Wrapper types are essentially the user-api. if this is an internal field autogenerated, then you should remove it (and auto-generate it when necessary).
if it's optional and in some cases it's useful to define it, then use a constructor to deal with the two cases.
|
Hey @dangirsh , this is a friendly bump. |
|
@PhilipVinc Unfortunately, I've recently started a non-Julia project that is consuming 100% of my time. I hope this initial attempt is a helpful start. |
See #77 .