Skip to content

feat: interference repair tool#1633

Merged
RobPasMue merged 30 commits intoblitzfrom
feat/fix_interference
Jan 24, 2025
Merged

feat: interference repair tool#1633
RobPasMue merged 30 commits intoblitzfrom
feat/fix_interference

Conversation

@smereu
Copy link
Copy Markdown
Contributor

@smereu smereu commented Jan 15, 2025

Description

This PR adds the the interference tool that was implemented in the API server but not in the python layer. One thing to notice is that this tool doesn't do tracking so the fix method will fail with the current API server. A pending API server PR is adding a try catch to allow lack of tracking to throw an exception.

Issue linked

Please mention the issue number or describe the problem this pull request addresses.

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)

RobPasMue and others added 3 commits January 2, 2025 15:49
The interference tool was exposed in the API server but not in python. One thing to notice is that this tool doesn't do tracking so the fix method will fail with the current API server. A pending PR is adding a try catch to allow lack of tracking to throw an exception.
@smereu smereu requested a review from a team as a code owner January 15, 2025 00:46
@github-actions github-actions bot added the enhancement New features or code improvements label Jan 15, 2025
@RobPasMue
Copy link
Copy Markdown
Member

Hi @smereu - thanks for the PR!

I changed the asignees.. the asignees should be the ones doing the PR, whereas the reviewers should be the ones reviewing. I assume you didn't want @b-matteo @jonahrb @jonahrb and myself doing the PR, but reviewing it.

I will take a look at it shortly!

Copy link
Copy Markdown
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.

Left some comments @smereu - some of them are suggestions which you can directly commit through the GitHub Web interface and then pull on your local branch. Other changes will require yourself to handle them. Feel free to ping me if you need any support!

Leaving as Request changes until comments are solved

Comment thread src/ansys/geometry/core/tools/problem_areas.py Outdated
Comment thread src/ansys/geometry/core/tools/problem_areas.py Outdated
Comment thread src/ansys/geometry/core/tools/problem_areas.py Outdated
Comment thread src/ansys/geometry/core/tools/problem_areas.py
Comment thread src/ansys/geometry/core/tools/repair_tools.py Outdated
@RobPasMue
Copy link
Copy Markdown
Member

RobPasMue commented Jan 15, 2025

Also - PyAnsys Geometry uses conventional commit styling for its PR names. You can see that this CI/CD stage failed here: https://github.com/ansys/pyansys-geometry/actions/runs/12779177188/job/35623345385?pr=1633

If you change the name of your PR accordingly, it should pass. For example, since this is a feature, it makes sense that the name of the PR is feat: interference repair tool. You can change the name of the PR easily from the GitHub web interface as well by clicking on the Edit button at the right of the PR title.

smereu and others added 3 commits January 15, 2025 10:17
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
@smereu smereu changed the title Interference repair tool feat: interference repair tool Jan 15, 2025
@smereu
Copy link
Copy Markdown
Contributor Author

smereu commented Jan 15, 2025

@RobPasMue Thanks for the review Roberto and apologies for silly mistakes :-). If there is anything outstanding, please drop me a note

Comment thread src/ansys/geometry/core/tools/repair_tools.py
Copy link
Copy Markdown
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.

2 small extra comments - related to protect_grpc. Once that's solved, this PR is ready to merge =)

Comment thread src/ansys/geometry/core/tools/problem_areas.py
@smereu
Copy link
Copy Markdown
Contributor Author

smereu commented Jan 20, 2025

@RobPasMue The test for fix interference should pass with the latest build but I still see it failing. Is there something I can do to check? Any other comment to be resolved?

Comment thread src/ansys/geometry/core/tools/repair_tools.py
@RobPasMue
Copy link
Copy Markdown
Member

Hi @smereu - I had to implement commit fc3b578 otherwise the check_type method couldn't run since it had no idea about what a Body object is (it was only being imported for type hint purposes).

Anyway, it looks like your tests are failing against Core Service... and my knowledge on that area is limited. Until it is fixed on the service or on the test itself, we can't merge it.

Copy link
Copy Markdown
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.

LGTM!

Comment thread src/ansys/geometry/core/tools/repair_tools.py Outdated
Comment thread src/ansys/geometry/core/tools/repair_tools.py
Comment thread src/ansys/geometry/core/tools/repair_tools.py
Comment thread src/ansys/geometry/core/tools/problem_areas.py
@RobPasMue RobPasMue enabled auto-merge (squash) January 24, 2025 08:02
@RobPasMue RobPasMue merged commit edac0da into blitz Jan 24, 2025
@RobPasMue RobPasMue deleted the feat/fix_interference branch January 24, 2025 08:18
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.

6 participants