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 'set_vertical_balance' method to DSheetPilingModel #182

Merged
merged 13 commits into from
Dec 4, 2024

Conversation

KentropDevelopment
Copy link
Contributor

@KentropDevelopment KentropDevelopment commented May 14, 2024

Fixes issue

Allows the user to set the parameters needed for checking the vertical balance in D-Sheet Piling.

It might be good to review if my changes are is consistent and correct regarding the following points:

  • In internal.py I created VerticalBalance and inhereted from DSeriesInlineMappedProperties.
  • I put the 'user version' of VerticalBalance in constructions.py, since the vertical balance settings belong to the construction, but not to one element or model specifically.
  • I added test_add_vertical_balance and gave it the marker unittest.

I am happy to receive feedback on my contribution.

Regards,
Daniël Kentrop

Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Collaborator

@VirginieTrompille VirginieTrompille left a comment

Choose a reason for hiding this comment

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

A test is missing to check that after setting the vertical balance properties they are correctly written in the *.shi file.
You can for example improve the existing test "test_run_sheet_model_acceptance_different_calculation_types" by setting the vertical balance properties to values different from the default values.

@KentropDevelopment
Copy link
Contributor Author

Hi Virginie,

Thanks for reviewing the pull request! I added setting the vertical balance properties to "test_run_sheet_model_acceptance_different_calculation_types" and changed the name of the other test like you suggested.

I'd like to hear your view on the 'internal' point. I can process any suggestions.

Only somehow the integration tests fail for pydantic v1 python 3.12. The errors occur in code I believe I did not affect. Do you have suggestions for solving this or is this another issue?

Kind regards,
Daniël

Copy link
Collaborator

@VirginieTrompille VirginieTrompille left a comment

Choose a reason for hiding this comment

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

I can't approve this pull request if all the tests are not OK. If you fix the merge conflict with the main, it is possible that all the tests will pass.

tests/models/dsheetpiling/test_dsheetpiling_acceptance.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Aug 28, 2024

@KentropDevelopment
Copy link
Contributor Author

The pydantic issue was fixed and I updated my changes accordingly and resolved the merge conflict that emerged in the meanwhile. Only one check fails, with this error:

Error output:
No Python at '"C:\hostedtoolcache\windows\Python\3.12.4\x64\python.exe'

Copy link

sonarcloud bot commented Dec 4, 2024

@VirginieTrompille VirginieTrompille merged commit 32dea98 into Deltares:master Dec 4, 2024
14 checks passed
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