Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage can create ensemble with state HAS_DATA when no data present #7046

Closed
eivindjahren opened this issue Jan 30, 2024 · 10 comments · Fixed by #7150
Closed

storage can create ensemble with state HAS_DATA when no data present #7046

eivindjahren opened this issue Jan 30, 2024 · 10 comments · Fixed by #7150
Assignees
Labels

Comments

@eivindjahren
Copy link
Contributor

eivindjahren commented Jan 30, 2024

The following fails:

def test_empty_ensemble_should_be_undefined():
    with open_storage("/tmp/storage", mode="w") as storage:
        experiment = storage.create_experiment(
            parameters=[
                GenKwConfig(
                    template_file="//tmp/tmp280u73a6/template_file",
                    transfer_function_definitions=[],
                    name="0",
                    forward_init=False,
                    output_file=None,
                )
            ],
            responses=[
                SummaryConfig(
                    name="A",
                    input_file="D",
                    keys=[],
                    refcase=set(),
                ),
            ],
        )
        ensemble = storage.create_ensemble(ensemble_size=1, experiment=experiment.id)
        assert ensemble.get_ensemble_state() == [RealizationStorageState.UNDEFINED]

The same issue reappears when you do create_ensemble with prior:

def test_creating_ensemble_from_empty_prior_results_in_parent_failure():
    with open_storage("/tmp/storage", mode="w") as storage:
        experiment = storage.create_experiment(
            parameters=[
                GenKwConfig(
                    template_file="//tmp/tmp280u73a6/template_file",
                    transfer_function_definitions=[],
                    name="0",
                    forward_init=False,
                    output_file=None,
                )
            ],
            responses=[
                SummaryConfig(
                    name="A",
                    input_file="D",
                    keys=[],
                    refcase=set(),
                ),
            ],
        )
        ensemble1 = storage.create_ensemble(ensemble_size=1, experiment=experiment.id)
        ensemble2 = storage.create_ensemble(
            ensemble_size=1, experiment=experiment.id, prior_ensemble=ensemble1
        )
        assert ensemble2.get_ensemble_state() == [
            RealizationStorageState.PARENT_FAILURE
        ]

This situation can be provoked from a config by setting

NUM_REALIZATIONS  100
ECLBASE BASE

and then not having any summary observations or history observations.

@frode-aarstad frode-aarstad self-assigned this Feb 14, 2024
@frode-aarstad
Copy link
Contributor

@eivindjahren are the tests above part of a branch about to be merged - or should I add similar tests to this PR?

@eivindjahren
Copy link
Contributor Author

eivindjahren commented Feb 14, 2024

@frode-aarstad You can just uncomment the lines marked with issue 7046 here:

# https://github.com/equinor/ert/issues/7046
# assert (
# ensemble.get_ensemble_state()
# == [RealizationStorageState.UNDEFINED] * ensemble_size
# )

@frode-aarstad
Copy link
Contributor

frode-aarstad commented Feb 14, 2024

The issue here is if we should allow construction of SummaryConfig with no keys. When reading summary and this is the case we throw an exception.

(https://github.com/equinor/ert/blob/6895be5119c0769fdd5899f75d6be571459df206/src/ert/config/summary_config.py)

@eivindjahren
Copy link
Contributor Author

The issue here is if we should allow construction of SummaryConfig with no keys. When reading summary and this is the case we throw an exception.

(https://github.com/equinor/ert/blob/6895be5119c0769fdd5899f75d6be571459df206/src/ert/config/summary_config.py)

Can the bug be triggered by putting SUMMARY "" in the config file?

@eivindjahren
Copy link
Contributor Author

It might be that all we need is to update the documentation of RealizationStorageState. I'll have a check.

@eivindjahren
Copy link
Contributor Author

eivindjahren commented Feb 15, 2024

Storage is quite inconsistent with what it considers "HAS_DATA". Usually, a forall quantifier in front of an empty collection is considered True (see https://en.wikipedia.org/wiki/Vacuous_truth). That is why all([]) == True.

However, when LocalEnsemble.response_configuration is empty, _all_responses_exist_for_realization returns False, but if LocalEnsemble.response_configuration contains an SummaryConfig without keys, you get True.

This is also inconsistent with _get_parameter_mask which uses

all(
     path / f"{parameter}.nc").exists()
     for parameter in self.experiment.parameter_configuration
)

@eivindjahren
Copy link
Contributor Author

eivindjahren commented Feb 15, 2024

I suggest we do the following: #7150 which enforces that SummaryConfig must have keys and changes the name and documentation of the existence checks in local_ensemble.

Also, I think the comment in _filter_response_configuration is incorrect as it dis produce a file, but that file actually ended up the triggering the bug #6974 so now that is a forward model failure. So now that SummaryConfig cannot have empty keys, I removed _filter_response_configuration as it does nothing.

@frode-aarstad
Copy link
Contributor

I think the proposed solution of forcing SummaryConfig to have keys is good and leads to the proposed simplifications to local_ensemble

@eivindjahren
Copy link
Contributor Author

eivindjahren commented Feb 16, 2024

After looking a bit closer at it, that solution seems to not be possible as it would break with current behavior that
summary keys from HISTORY_OBSERVATION are added to the summary config if missing.

if summary_key not in response_config.keys:
response_config.keys.append(summary_key)

@eivindjahren
Copy link
Contributor Author

It seems like this issue is less problematic than first anticipated so we can lower the severity of the bug at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants