Skip to content

Commit

Permalink
Remove _check_previous_required_observation method (facebook#1999)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: facebook#1999

This method is redundant and checks for a state that we should never reach. In the process of updating to GenerationNode based GS, maintaining this method would entail keeping track of previous nodes. The tech debt of doing so outweighs the potential reward of keeping the function. Also I think since use_update has been deprecated this function is also less relevant.

Reviewed By: lena-kashtelyan

Differential Revision: D51420534

fbshipit-source-id: a17a48acc811579d15012cb44c1ee8aca2941428
  • Loading branch information
mgarrard authored and facebook-github-bot committed Nov 29, 2023
1 parent 110a0e2 commit 14a5615
Showing 1 changed file with 1 addition and 20 deletions.
21 changes: 1 addition & 20 deletions ax/modelbridge/generation_strategy.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from ax.core.generator_run import GeneratorRun
from ax.core.observation import ObservationFeatures
from ax.core.utils import extend_pending_observations
from ax.exceptions.core import DataRequiredError, NoDataError, UserInputError
from ax.exceptions.core import DataRequiredError, UserInputError
from ax.exceptions.generation_strategy import GenerationStrategyCompleted

from ax.modelbridge.base import ModelBridge
Expand Down Expand Up @@ -457,7 +457,6 @@ def _fit_current_model(self, data: Optional[Data]) -> None:

self._curr.fit(experiment=self.experiment, data=data, **model_state_on_lgr)
self._model = self._curr.model_spec.fitted_model
self._check_previous_required_observation(data=data)

def _maybe_move_to_next_step(self, raise_data_required_error: bool = True) -> bool:
"""Moves this generation strategy to next step if the current step is completed,
Expand Down Expand Up @@ -530,21 +529,3 @@ def _get_model_state_from_last_generator_run(self) -> Dict[str, Any]:
)

return model_state_on_lgr

def _check_previous_required_observation(self, data: Data) -> None:
previous_step_req_observations = (
self._curr.index > 0
and self._steps[self._curr.index - 1].min_trials_observed > 0
)
# If previous step required observed data, we should raise an error even if
# enough trials were completed. Such an empty data case does indicate an
# invalid state; this check is to improve the experience of detecting and
# debugging the invalid state that led to this.
if data.df.empty and previous_step_req_observations:
raise NoDataError(
f"Observed data is required for generation node {self._curr.node_name},"
f"(model {self._curr.model_to_gen_from_name}), but fetched data was "
"empty. Something is wrong with experiment setup -- likely metrics "
"do not implement fetching logic (check your metrics) or no data "
"was attached to experiment for completed trials."
)

0 comments on commit 14a5615

Please sign in to comment.