Skip to content

feat: extract fluid volume from solid#1306

Merged
smereu merged 18 commits into
mainfrom
feat/smereu/volume_extraction
Jul 16, 2024
Merged

feat: extract fluid volume from solid#1306
smereu merged 18 commits into
mainfrom
feat/smereu/volume_extraction

Conversation

@smereu
Copy link
Copy Markdown
Contributor

@smereu smereu commented Jul 15, 2024

Description

This PR provides the functionality to extract a flow volume from a list of sealing faces or edge loops

Issue linked

#1305

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)

smereu added 6 commits July 12, 2024 12:36
Expose APIs for volume extraction from sealing faces or sealing edge loops
More files for volume extraction exposure
Fix code style from precommit check
fix file name for test
fix typos
Add geometry update if bodies created
@smereu smereu requested a review from a team as a code owner July 15, 2024 13:43
@smereu smereu requested a review from umutsoysalansys July 15, 2024 13:43
@smereu smereu self-assigned this Jul 15, 2024
@smereu smereu requested a review from ansbdickens July 15, 2024 13:43
@github-actions github-actions Bot added the enhancement New features or code improvements label Jul 15, 2024
RobPasMue
RobPasMue previously approved these changes Jul 16, 2024
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.

Some minor suggestions. Overall, LGTM!

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

There are problems with the unstable image and the stable image does not include these features so.. until that is solved we won't be able to merge the PR

@RobPasMue RobPasMue linked an issue Jul 16, 2024 that may be closed by this pull request
code review

Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
smereu and others added 3 commits July 16, 2024 08:22
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
Respond to code review
@smereu
Copy link
Copy Markdown
Contributor Author

smereu commented Jul 16, 2024

@RobPasMue A few comments, in case they could help someone with similar questions

  1. I think I don't need the decorator @reset_tessellation_cache since I am "force updating" the design which I assume will also bring the facets. Let me know if this is correct
  2. There is a decorator to ensure_design_is_active whose meaning I am not sure. If we support multiple design the design must be always passed or inferred from the passed geometry. I may not be clear on how (or how well :-) ) we support multiple desins

@smereu smereu enabled auto-merge (squash) July 16, 2024 13:41
@RobPasMue RobPasMue disabled auto-merge July 16, 2024 13:42
@RobPasMue
Copy link
Copy Markdown
Member

Hi @smereu - point taken! :) @reset_cache is not needed on your PR. @ensure_design_is_active might be interesting. Let me check in depth your PR.

Also, I left some additional comments which I think weren't addressed, I'll do the modifications myself. :)

Nonetheless, the PR won't be able to be merged yet. There seems to be some issues in the implementation. Looking into them as well

@smereu
Copy link
Copy Markdown
Contributor Author

smereu commented Jul 16, 2024

Thanks Roberto and apologies for giving you more work :-(. ensure_design_is_active should not be needed here because we are inferring the design from the geometry. I was not clear why it is needed in some other methods where I saw it but that is a topic for discussion separate from this PR.
As you might have seen, there was an API server commit done for the web app last Friday that broke almost everything and I fixed it last evening. Apart from that though, I am not sure what is wrong with my methods. I tested them last week and they were all working fine using my local stubs. I should have the updated stubs now but they are failing for me locally. Do ping me if you have something to discuss. Thx again

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 84.12698% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.01%. Comparing base (1df9f11) to head (658b638).

Files Patch % Lines
src/ansys/geometry/core/tools/prepare_tools.py 82.22% 8 Missing ⚠️
src/ansys/geometry/core/modeler.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1306      +/-   ##
==========================================
- Coverage   92.07%   92.01%   -0.07%     
==========================================
  Files          85       86       +1     
  Lines        6602     6660      +58     
==========================================
+ Hits         6079     6128      +49     
- Misses        523      532       +9     

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

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!

@smereu smereu merged commit e78525e into main Jul 16, 2024
@smereu smereu deleted the feat/smereu/volume_extraction branch July 16, 2024 19:00
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.

Volume extraction exposure

3 participants