Skip to content

Conversation

rmanno91
Copy link
Collaborator

No description provided.

Comment on lines 41 to 44
default_value: float | str | None = Field(
default=None,
title="Default Value",
description="The default value of the independent parameter.",

Choose a reason for hiding this comment

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

Why can this be str?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in eng data this could be "Program Controlled". I have added it to handle that case
image

Comment on lines 49 to 54
upper_limit: str | float | None = Field(
default=None, title="Upper Limit", description="Upper limit of the independent parameter."
)
lower_limit: str | float | None = Field(
default=None, title="Lower Limit", description="Lower limit of the independent parameter."
)

Choose a reason for hiding this comment

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

Same here, why can this be a str?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as for before

quantized: bool | None = Field(
default=None, title="Quantized", description="Whether the data is quantized."
)
extrapolation_type: str | None = Field(

Choose a reason for hiding this comment

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

extrapolation_type on InterpolationOptions, looks a bit strange imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is not great to me eirther, I followed Engineering Data reference
image
but definitely keen to change for better naming if you have better ideas.

name: Literal["Coefficient of Thermal Expansion"] = Field(
default="Coefficient of Thermal Expansion", repr=False, frozen=True
)
supported_packages: SupportedPackage = Field(

Choose a reason for hiding this comment

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

Maybe this is already terminology in this repo, but maybe a better name would be supported backends or smth else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I followed the same as defined before, but I think we could change those names.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Package is the terminology we use in the MatBU - referring to CAE Package

Comment on lines +95 to +97
def get_model_by_name(self, model_name: str) -> List[MaterialModel]:
"""Get the material model or models with a given model name."""
return [model for model in self.models if model.name.lower() == model_name.lower()]

Choose a reason for hiding this comment

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

Name is not unique? So returning a list is expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in the case of usermaterial apparently you can have multiple definition of those i.e. multiple material model with the same name. But so far this should be the only case when this happens.

@@ -26,5 +26,6 @@
class SupportedPackage(Flag):
"""Provides the enum representing the packages supported by the Material Manager."""

MATML = auto()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to add this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed for the time being

default="", title="Name", description="The name of the model qualifier.", frozen=True
)
value: str = Field(
default="", title="Value", description="The value of the model qualifier.", frozen=True
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to use frozen here?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Is the intention to make qualifiers immutable?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want the qualifier to be immutable one initialized.

from ansys.materials.manager.material import Material


class CoefficientofThermalExpansionIsotropic(MaterialModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class CoefficientofThermalExpansionIsotropic(MaterialModel):
class CoefficientOfThermalExpansionIsotropic(MaterialModel):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would keep it as it is to do not complicate the logic in the locate function. The name of the class is used to call it from the matml name of the property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but it is doing different stuff.

title="Model Qualifiers",
description="Model qualifiers for the strain limits isotropic model.",
)
von_mises: list[float] = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
von_mises: list[float] = Field(
von_mises_stress: list[float] = Field(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is strain limit.

from ansys.materials.manager.material import Material


class ZeroThermalStrainReferenceTemperatureIsotropic(MaterialModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this matter whether it is Isotropic or Orthotropic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nothing really, it is just to avoid an if else in the initialization of the material model in the material_from_matml.py file. I would keep it as it is for the time being. We can work on it later

Choose a reason for hiding this comment

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

should these imports first live in src/ansys/materials/manager/_models/_material_models/__init__.py and then here we import from there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in #359

Comment on lines +54 to +58
material_property: str | None = Field(
default=None,
title="Material Property",
description="The material property for the material model.",
)

Choose a reason for hiding this comment

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

what's this needed for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is something I have seen in the xml file that is used in serveral places. I am not entirely sure of what it is doing. In my broad understanding it is a string used to link two parameters together like "Color" and "Appereance" or CTE and zero thermal strain

title="Zero Thermal Strain Reference Temperature",
description="The reference temperature for zero thermal strain.",
)
material_property: Literal["Coefficient of Thermal Expansion"] = Field(

Choose a reason for hiding this comment

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

material_property needs to be renamed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in this case need to be assigned.

title="Zero Thermal Strain Reference Temperature",
description="The reference temperature for zero thermal strain.",
)
material_property: Literal["Coefficient of Thermal Expansion"] = Field(

Choose a reason for hiding this comment

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

material_property needs to be renamed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as for before

Comment on lines +45 to +50
material_property: str = Field(
default="Appearance",
title="Material Property",
description="The material property associated with this model.",
frozen=True,
)

Choose a reason for hiding this comment

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

needs to be updated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needs to be assigned.

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.

5 participants