Add option to use t0 embedding features #505
Conversation
| embedding_dim=None, | ||
| include_sun=False, | ||
| include_time=True, | ||
| t0_embedding_dim=3, |
There was a problem hiding this comment.
This might be my lack of understanding, but is it obvious how know the t0_embedding_dim from the t0_embedding embedding (below)?
There was a problem hiding this comment.
Well in the function docstring it explains that the embedding_dim parameter is for the location ID. We could rename embedding_dim->loc_embedding_dim or something similar to be more explicit. But for that we'd need to migrate all our production models
There was a problem hiding this comment.
Oh sorry, I think I misunderstood your question. You mean how can you know what the t0_embedding_dim needs to be given t0_embedding config? That info is in data-sampler.
Basically:
t0_embedding_dim = sum([1 if e=="linear" else 2 for e in t0_embedding.embeddings])
There was a problem hiding this comment.
ah i see, could you put this comment in the docstrings? (Or if there somewhere else more suitable)
There was a problem hiding this comment.
This feels like bloat here since it is explained in full in data-sampler
There was a problem hiding this comment.
Maybe a compromise is to put the explanation in the example config
|
Other than my questions, this looks good and clear |
dfulu
force-pushed
the
use_t0_embedding
branch
from
6c004cf to
f6b9a10
Compare
dfulu
force-pushed
the
use_t0_embedding
branch
from
f6b9a10 to
d076bc4
Compare
Pull Request
Description
This adds the option to use the t0-embedding features which were added to data-sampler in openclimatefix/ocf-data-sampler#385 in PVNet
Checklist: