Skip to content

Added water boundary for multiline or interpolation - #49

Open
EleniSmyrniou wants to merge 141 commits into
mainfrom
water_boundaries
Open

Added water boundary for multiline or interpolation#49
EleniSmyrniou wants to merge 141 commits into
mainfrom
water_boundaries

Conversation

@EleniSmyrniou

Copy link
Copy Markdown
Collaborator

No description provided.

@EleniSmyrniou EleniSmyrniou self-assigned this Jul 5, 2023
Comment thread stem/IO/kratos_water_boundaries_io.py
from stem.water_boundaries import WaterBoundary, PhreaticMultiLineBoundary, InterpolateLineBoundary, WaterBoundaryParameters


class KratosWaterBoundariesIO:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cna you add the docstring for this class and init

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

missing attributes :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I really don't understand what is missing

Comment thread stem/IO/kratos_water_boundaries_io.py Outdated

def __water_boundary_dict(self, name: str, type: str, water_boundary: WaterBoundaryParameters) -> Dict[str, Any]:
"""
Creates a dictionary containing the water boundary parameters

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you add the types in the docstring and style the Return like:

    """
    Creates a dictionary containing the material parameters for a UDSM material. The UDSM parameters are moved to
    the UMAT_PARAMETERS key, as this can be recognized by Kratos.

    Args:
        - material (:class:`stem.soil_material.SoilConstitutiveLawABC`): soil constitutive law object containing the material parameters for UDSM

    Returns:
        - Dict[str, Any]: dictionary containing the material parameters
    """

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same for rest of the functions

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this comment is not resolved yet

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is also resolved

Comment thread stem/IO/kratos_water_boundaries_io.py Outdated
def __init__(self, domain: str):
self.domain = domain

def __water_boundary_dict(self, name: str, type: str, water_boundary: WaterBoundaryParameters) -> Dict[str, Any]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def __water_boundary_dict(self, name: str, type: str, water_boundary: WaterBoundaryParameters) -> Dict[str, Any]:
def __create_water_boundary_dict(self, name: str, type: str, water_boundary: WaterBoundaryParameters) -> Dict[str, Any]:

Comment thread stem/IO/kratos_water_boundaries_io.py Outdated

"""
if isinstance(water_boundary, PhreaticMultiLineBoundary):
parameters: Dict[str, Any] = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you add a separate private function per boundary? to keep it a bit more overseeable.

e.g.:

if isinstance(water_boundary, PhreaticMultiLineBoundary):     
    self.__create_phreatic_multi_line_boundary_dict

Comment thread stem/IO/kratos_water_boundaries_io.py Outdated
Returns: dictionary containing the water boundary parameters

"""
return self.__water_boundary_dict(water_boundary.name,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return self.__water_boundary_dict(water_boundary.name,
return self.__create_water_boundary_dict(water_boundary.name,

Comment thread stem/water_boundaries.py Outdated
y_coordinates: List[float] = field(default_factory=lambda: [0.0])
z_coordinates: List[float] = field(default_factory=lambda: [0.0])
specific_weight: float = 9.81
water_pressure: float = 0.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

water pressure is not in the docstring

Comment thread stem/water_boundaries.py Outdated


"""
surfaces_assigment: List[str] = field(default_factory=lambda: [""])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think the surface assignment will be managed in the modelparts. In this file I propose to only put the parameters as present in the projectparameters.json

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

in the future, we should add some helper functions in model.py, which should handle this. But I would say thats a problem for later

Comment thread stem/water_boundaries.py Outdated


"""
x_coordinates: List[float] = field(default_factory=lambda: [0.0])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the parameters here should not be given as a default, as I think its important that the user actively thinks about what he/she wants

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

except the z_coordinates, that is only required in 3D, so this can be checked in the post_init if z_coordinates are present in 3D cases

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes the z coordinates are checked in the post init

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that there are default values in the parent class. If a default value is defined in the parent class then everything else needs to have default values. This has to do with the ordering of the variables :/ I will make everything undefault but this is also not ideal

Comment thread stem/water_boundaries.py Outdated

"""
surfaces_assigment: List[str] = field(default_factory=lambda: [""])
is_fixed: bool = True

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these parameters should not be given a default. As wrong stuff can happen in 3D situations, when the user keeps it on the default values

Comment thread stem/water_boundaries.py Outdated

"""

def __init__(self, water_boundary: Union[InterpolateLineBoundary, PhreaticMultiLineBoundary], name: str):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, water_boundary: Union[InterpolateLineBoundary, PhreaticMultiLineBoundary], name: str):
def __init__(self, water_boundary_parameters: Union[InterpolateLineBoundary, PhreaticMultiLineBoundary], name: str):

morettid and others added 30 commits July 27, 2023 13:45
Co-authored-by: aronnoordam <51492202+aronnoordam@users.noreply.github.com>
Adding geometry for load application and fixes in testing for model
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.

4 participants