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: Add dataclass DerivedNamedStratigraphy #468

Merged
merged 1 commit into from
Feb 13, 2024
Merged

CLN: Add dataclass DerivedNamedStratigraphy #468

merged 1 commit into from
Feb 13, 2024

Conversation

janbjorge
Copy link
Contributor

@janbjorge janbjorge commented Feb 13, 2024

Adressess: #457

@janbjorge janbjorge self-assigned this Feb 13, 2024
Comment on lines -175 to -176
def __post_init__(self) -> None:
logger.info("Ran __post_init__")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems unnecessary to me.

@janbjorge janbjorge marked this pull request as ready for review February 13, 2024 11:40
Comment on lines 185 to 186
if not no_start_or_missing_name and rv.name != "name":
rv.alias.append(name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be moved inside the above function block by usage of walrus op. But gets unreadable.

@janbjorge janbjorge changed the title CLN: Add dataclass DerivedNameAndStratigraphy CLN: Add dataclass DerivedNamedStratigraphy Feb 13, 2024
metadata: dict = field(default_factory=dict)
name: str = field(default="")
specs: dict = field(default_factory=dict)
subtype: str = field(default="")
Copy link
Member

Choose a reason for hiding this comment

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

subtype...?

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 PR is split out off #469 looks like i missed this one. Typically we get something like below. But for now, it seems like it not used/propagated.

DerivedObjectDescriptor(
                subtype="CPGridProperty",
                classname="cpgrid_property",

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.

As I read it, should be 1:1 with existing functionality for doing lookup in the stratigraphic column and replacing given name with derived actual name.

@janbjorge
Copy link
Contributor Author

As I read it, should be 1:1 with existing functionality for doing lookup in the stratigraphic column and replacing given name with derived actual name.

Correct, there should be no new logical changes here. Expect for that we now use a dataclass insted of a dict to communicate between functions/classes.

@janbjorge janbjorge merged commit 857b597 into equinor:main Feb 13, 2024
13 checks passed
@janbjorge janbjorge deleted the add-DerivedNameAndStratigraphy-dataclass branch February 13, 2024 16:21
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