Skip to content

Commit

Permalink
Clean up unneccesary todos in _get_model_state_from_last_generator_run
Browse files Browse the repository at this point in the history
Summary:
Since we decided to keep a 1 fitted model : 1 node relationship the note about this being uncompatable with GenNodes is irrelevant now, easy compatability :) 

Also removed the todo about moving to gennodes - i think it's okay where it is, but if we want to move _fit_current_model and _get_model_state_from_last_generator_run to GenerationNode, that doesn't seem unreasonable and we can keep the todo

Reviewed By: lena-kashtelyan

Differential Revision: D57444926
  • Loading branch information
mgarrard authored and facebook-github-bot committed May 22, 2024
1 parent 5a19dca commit 2de4ae2
Showing 1 changed file with 0 additions and 7 deletions.
7 changes: 0 additions & 7 deletions ax/modelbridge/generation_strategy.py
Original file line number Diff line number Diff line change
Expand Up @@ -887,11 +887,6 @@ def _maybe_transition_to_next_node(

def _get_model_state_from_last_generator_run(self) -> Dict[str, Any]:
lgr = self.last_generator_run
# NOTE: This will not be easily compatible with `GenerationNode`;
# will likely need to find last generator run per model. Not a problem
# for now though as GS only allows `GenerationStep`-s for now.
# Potential solution: store generator runs on `GenerationNode`-s and
# split them per-model there.
model_state_on_lgr = {}
# Need to check if model_spec_to_gen_from is none to account for
# ExternalGenerationNodes which leverage models from outside Ax.
Expand All @@ -910,8 +905,6 @@ def _get_model_state_from_last_generator_run(self) -> Dict[str, Any]:

if grs_equal and lgr._model_state_after_gen:
if self.model or isinstance(model_on_curr, ModelRegistryBase):
# TODO[drfreund]: Consider moving this to `GenerationStep` or
# `GenerationNode`.
model_cls = (
self.model.model.__class__
if self.model is not None
Expand Down

0 comments on commit 2de4ae2

Please sign in to comment.