Bnb/exo refactor #167
Bnb/exo refactor #167
Conversation
bnb32
force-pushed
the
bnb/exo_refactor
branch
3 times, most recently
from
ddd63b9 to
9eec5c8
Compare
… forward pass to use dictionary of features. added multi exo tests in test_forward_pass.py
…ence. t_agg_factor added to ExoExtract. Prelim tests working for single exo feature.
…rs, and extracting exo data.
…. Both generalized for multiple exogenous features
…thout exo data defined in abstract single model and removed from base. forward pass tests working.
…Gan/Sup3rCondMom/Sup3rGanDC now works with/without mid network exo injection. Transpose method added to MultiStepGan to match input data shape with model.input_dims. This should accomplish the same as SpatialThenTemporal/TemporalThenSpatial models in addition to a st model + spatial + st model, for example.
…sts modified to handle new exogenous_data input format.
bnb32
force-pushed
the
bnb/exo_refactor
branch
3 times, most recently
from
3cac240 to
0450f21
Compare
bnb32
force-pushed
the
bnb/exo_refactor
branch
3 times, most recently
from
f729042 to
42553f5
Compare
bnb32
force-pushed
the
bnb/exo_refactor
branch
7 times, most recently
from
207f4af to
854e738
Compare
bnb32
force-pushed
the
bnb/exo_refactor
branch
from
854e738 to
fb6787f
Compare
There was a problem hiding this comment.
What a burly PR! Overall i really like what you've done to bring the exo data handing into the base GAN model architecture. I think the config structure of the exo_data stuff makes sense, my main comment is just to put all of the initialization logic into a class and then point to that class for how to setup a valid config. Putting these special endless kwarg data structures into separate classes will make sure we have a dedicated place for documentation and an easier time with future developments.
| steps = [step for step in exogenous_data[feature]['steps'] | ||
| if step['model'] == model_step] | ||
| if steps: | ||
| model_step_exo[feature] = {'steps': steps} |
There was a problem hiding this comment.
It might be worth defining a separate data structure for the exogenous_data dictionary, especially if it defines the data structure more explicitly thereby helping users decipher how to set up the config.
Consider a couple of classes: class SingleExoDataStep(combine_type, model, data), class ExoData(steps)... Would be extra good if ExoData could be init from a config file directly OR a collection of SingleExoDataStep instances. Then the ExoData class would have methods to retrieve various bits of the data.
Not totally sure how this would fit into the refactored fwp config as i haven't gotten there yet.
There was a problem hiding this comment.
This is a cool idea. Maybe better to leave for another PR so this initial big refactor can be merged first?
There was a problem hiding this comment.
Yeah that's fine, i think separate data struct classes and removing the spatial/temporal model are the two pieces left "todo", right?
There was a problem hiding this comment.
Just the struct classes, if were ok with keeping the SpatialThenTemporalBase used only by the Solar model.
bnb32
force-pushed
the
bnb/exo_refactor
branch
5 times, most recently
from
a3e1885 to
4f01b30
Compare
bnb32
force-pushed
the
bnb/exo_refactor
branch
from
4f01b30 to
0e874c6
Compare
Moved most of the dict parsing methods from forward_pass into ExogenousDataHandler. Do you think this (and methods in model classes) could be improved with data structure classes? |
bnb32
force-pushed
the
bnb/exo_refactor
branch
2 times, most recently
from
dbc2635 to
0958306
Compare
bnb32
force-pushed
the
bnb/exo_refactor
branch
from
0958306 to
d78bc08
Compare
Changed exogenous data extraction to use a dictionary specifying model and combine_type (input/layer/output) for each use of exogenous data. Refactored all uses of exo data in models/tests/fwp according to this format.
Added SZA exogenous data handler for future use in solar models.
Combined basic gan models and wind gan models, including conditional models, which revert to basic models without exogenous data input.