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

feat: driving dimensions #1340

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Conversation

umutsoysalansys
Copy link
Contributor

@umutsoysalansys umutsoysalansys commented Aug 2, 2024

Description

Bringing driving dimensions/ geometry parametrization to pyansys-geometry.

image

Issue linked

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate unit tests.
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved to the PR if any.
  • I have assigned this PR to myself.
  • I have added the minimum version decorator to any new backend method implemented.
  • I have made sure that the title of my PR follows Conventional commits style (e.g. feat: extrude circle to cylinder)

@github-actions github-actions bot added the enhancement New features or code improvements label Aug 2, 2024
@umutsoysalansys umutsoysalansys changed the title Feat/driving dimensions feat: driving dimensions Aug 2, 2024
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 16 lines in your changes missing coverage. Please review.

Project coverage is 91.68%. Comparing base (b1d6271) to head (0fae95a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ansys/geometry/core/parameters/parameters.py 73.68% 10 Missing ⚠️
src/ansys/geometry/core/designer/design.py 71.42% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1340      +/-   ##
==========================================
- Coverage   91.84%   91.68%   -0.16%     
==========================================
  Files          86       88       +2     
  Lines        6928     6988      +60     
==========================================
+ Hits         6363     6407      +44     
- Misses        565      581      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@umutsoysalansys umutsoysalansys self-assigned this Aug 2, 2024
@RobPasMue
Copy link
Member

Let's discuss this later @umutsoysalansys but I wouldn't have it as an independent module of the whole "design" structure. I think driving dimensions should be a property of bodies...

@umutsoysalansys umutsoysalansys marked this pull request as ready for review October 16, 2024 02:25
@umutsoysalansys umutsoysalansys requested a review from a team as a code owner October 16, 2024 02:25
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Overall looks good @umutsoysalansys! I left some enhancement suggestions. Main missing point is an example. We need examples that demonstrates how to use these new features. This is part of the requested actions for any new implementation. Otherwise users won't know how to make use of it.

src/ansys/geometry/core/designer/design.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/design.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/design.py Show resolved Hide resolved
src/ansys/geometry/core/parameters/parameter.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/parameters/parameter.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/parameters/parameter.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/design.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/design.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/design.py Outdated Show resolved Hide resolved
Comment on lines 2675 to 2677
def test_design_parameters(modeler: Modeler):
"""Test the design parameters functionality."""

Copy link
Member

Choose a reason for hiding this comment

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

Seems like the test is failing - do you know the reason? There seems to be some kind of issue with the server apparently... We are using the latest proto and servers to check this so... maybe a pending PR on the server side?

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Blocking merge until comments are solved/implemented and the example is added. Also, testing needs to pass.

PipKat
PipKat previously approved these changes Oct 23, 2024
Copy link
Member

@PipKat PipKat left a comment

Choose a reason for hiding this comment

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

Reviewed doc only to ensure it meets the PyAnsys style guidelines

@PipKat
Copy link
Member

PipKat commented Oct 23, 2024 via email

umutsoysalansys and others added 13 commits November 13, 2024 08:21
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
@umutsoysalansys umutsoysalansys marked this pull request as draft November 13, 2024 14:37
auto-merge was automatically disabled November 13, 2024 14:37

Pull request was converted to draft

umutsoysalansys and others added 2 commits November 13, 2024 08:37
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
@umutsoysalansys umutsoysalansys marked this pull request as ready for review November 13, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants