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

CLN: Use AllowedContent to deduce if field is composite #477

Merged
merged 3 commits into from
Feb 27, 2024
Merged

CLN: Use AllowedContent to deduce if field is composite #477

merged 3 commits into from
Feb 27, 2024

Conversation

janbjorge
Copy link
Contributor

@janbjorge janbjorge commented Feb 16, 2024

Followup: #474

@janbjorge janbjorge self-assigned this Feb 16, 2024
@janbjorge janbjorge changed the title Combine contents required dict with allow contents model CLN: Use AllowedContent to deduce if field is composite Feb 16, 2024

# This setting sets the FMU context for the output. If detected as a non-fmu run,
# the code will internally set actual_context=None
ALLOWED_FMU_CONTEXTS: Final = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsued(?)

CONTENTS_REQUIRED: Final = {
"fluid_contact": {"contact": True},
"field_outline": {"contact": False},
"field_region": {"id": True},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor bug discovered, the fields property and seismic should have been a part of this. (Confirmed with @jcrivenaes )

Copy link
Contributor Author

@janbjorge janbjorge Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug, but will keep it as is for now. A FutureWarning has been added.

@janbjorge janbjorge changed the title CLN: Use AllowedContent to deduce if field is composite BUG: Use AllowedContent to deduce if field is composite Feb 16, 2024
@janbjorge janbjorge changed the title BUG: Use AllowedContent to deduce if field is composite CLN: Use AllowedContent to deduce if field is composite Feb 16, 2024
SCHEMA: Final = (
"https://main-fmu-schemas-prod.radix.equinor.com/schemas/0.8.0/fmu_results.json"
)
VERSION: Final = "0.8.0"
SOURCE: Final = "fmu"


class ValidationError(ValueError, KeyError):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad merge? ValidationError is defined in two places.

@@ -71,3 +71,39 @@ class AllowedContent(BaseModel):
relperm: None = Field(default=None)
lift_curves: None = Field(default=None)
transmissibilities: None = Field(default=None)

@staticmethod
def requires_additional_input(field: str) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can go away once we make a switch "further up", cheep workaround for now.

Copy link
Member

@perolavsvendsen perolavsvendsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the tests, I think we should be nicely covered. See the general UX comment regarding the warning texts, which should be more tailored towards those who needs to read and understand them.

Copy link
Member

@perolavsvendsen perolavsvendsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🤷‍♂️

@janbjorge janbjorge merged commit 0657ced into equinor:main Feb 27, 2024
13 checks passed
@janbjorge janbjorge deleted the combine-contents-required-dict-with-allow-contents-model branch February 27, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants