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

Add elasticity and density #82

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Conversation

ipg-jsc
Copy link
Contributor

@ipg-jsc ipg-jsc commented Sep 4, 2024

Describe your changes

As discussed in the meeting on 04.09.2024:

  • Add elasticity properties in form of Young's modulus and Poisson's ratio to material example
  • Add density

Issue ticket number and link

Closes #58, closes #60

Checklist before requesting a review

  • I have performed a self-review of my code/documentation.
  • My changes generate no new warnings during the documentation generation.

@ipg-jsc ipg-jsc marked this pull request as ready for review September 4, 2024 14:54
Copy link
Collaborator

@ClemensLinnhoff ClemensLinnhoff left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.
The only thing I am not sure about is which fields should be required. But I propose to go over all fields in the end and decide if they should be required or not.

@ClemensLinnhoff
Copy link
Collaborator

@AsamDiegoSanchez @drsftx73 @KimuraDIVP could you do an additional formal review so we have the full support of all relevant CCB members?

@ipg-jsc
Copy link
Contributor Author

ipg-jsc commented Sep 12, 2024

@ClemensLinnhoff have you had anything specific in mind on how to add the source field to properties that are currently a single key-value pair?

I could imagine going for @DavidJRitter904's approach in #86 and call the higher level key density_data.
For the sake of consistency I would also change elasticityelasticity_data.

    "elasticity_data": {
        "youngs_modulus": 70e9,
        "poissons_ratio": 0.35,
        "source": "internet: https://en.wikipedia.org/wiki/Aluminium"
    },
    "density_data": {
        "density": 2699.0,
        "source": "internet: https://en.wikipedia.org/wiki/Aluminium"
    },

@ClemensLinnhoff
Copy link
Collaborator

@ClemensLinnhoff have you had anything specific in mind on how to add the source field to properties that are currently a single key-value pair?

I could imagine going for @DavidJRitter904's approach in #86 and call the higher level key density_data. For the sake of consistency I would also change elasticityelasticity_data.

    "elasticity_data": {
        "youngs_modulus": 70e9,
        "poissons_ratio": 0.35,
        "source": "internet: https://en.wikipedia.org/wiki/Aluminium"
    },
    "density_data": {
        "density": 2699.0,
        "source": "internet: https://en.wikipedia.org/wiki/Aluminium"
    },

I think this is a very good solution! I originally thought about something like this:

"density": {
    "value": 2699.0,
    "source": "internet: https://en.wikipedia.org/wiki/Aluminium"
},

But I actually like your approach better.

@ClemensLinnhoff ClemensLinnhoff mentioned this pull request Sep 12, 2024
3 tasks
@ClemensLinnhoff ClemensLinnhoff merged commit 19f6ee5 into main Sep 12, 2024
2 checks passed
@ClemensLinnhoff ClemensLinnhoff deleted the 58-density-60-elasticity branch October 10, 2024 06:22
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.

Define elasticity in the material properties Add Density of materials to material properties
3 participants