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

Geometry enhancements #583

Merged
merged 17 commits into from
Oct 8, 2021
Merged

Conversation

vhirtham
Copy link
Collaborator

@vhirtham vhirtham commented Oct 6, 2021

Changes

Apply some small enhancements to the Geometry class. Most of them help to improve the readability/understandability of the new tutorials.

  • create Geometry from a profile and extrusion length
  • __init__ also accepts groove types
  • add k3d as backend option to plot

Move to separate PR / issue

  • support unit strings in interfaces
  • OPTIONAL: Set pint wrapper to strict
  • Add new triangulation algorithm (found one that works for all polygon types)

Checks

  • updated CHANGELOG.rst
  • updated tests
  • updated doc/
  • update example/tutorial notebooks
  • update manifest file

@vhirtham vhirtham added units unit handling and pint geometry visualization plotting and related functions labels Oct 6, 2021
@vhirtham vhirtham self-assigned this Oct 6, 2021
weldx/geometry.py Outdated Show resolved Hide resolved
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #583 (2736c61) into master (e011e4e) will decrease coverage by 0.15%.
The diff coverage is 70.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
- Coverage   94.83%   94.67%   -0.16%     
==========================================
  Files          93       93              
  Lines        5745     5768      +23     
==========================================
+ Hits         5448     5461      +13     
- Misses        297      307      +10     
Impacted Files Coverage Δ
weldx/geometry.py 97.89% <57.89%> (-1.17%) ⬇️
weldx/visualization/k3d_impl.py 78.68% <76.47%> (+0.02%) ⬆️
weldx/visualization/matplotlib_impl.py 90.78% <100.00%> (+0.12%) ⬆️
weldx/welding/groove/iso_9692_1.py 99.51% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e011e4e...2736c61. Read the comment docs.

weldx/geometry.py Outdated Show resolved Hide resolved
@vhirtham
Copy link
Collaborator Author

vhirtham commented Oct 7, 2021

Had a really annoying circular dependency in the docs only. It's gone now, but I might have overdone it at someplace. So if you see any unnecessary changes to the imports, let me know.

Most of the uncovered lines are plot-related and will be covered once the new tutorials go online. So I think there is no need for additional testing. However, I will add a short and simple test case for the groove support, just to have that covered by the basic tests.

@vhirtham vhirtham marked this pull request as ready for review October 7, 2021 12:35
@vhirtham vhirtham requested a review from CagtayFabry October 7, 2021 12:36
@vhirtham vhirtham removed the units unit handling and pint label Oct 7, 2021
@CagtayFabry
Copy link
Member

Had a really annoying circular dependency in the docs only. It's gone now, but I might have overdone it at someplace. So if you see any unnecessary changes to the imports, let me know.

Most of the uncovered lines are plot-related and will be covered once the new tutorials go online. So I think there is no need for additional testing. However, I will add a short and simple test case for the groove support, just to have that covered by the basic tests.

imports look better now I guess (removing top level imports) 👍

did you do anything specific to fix this error? https://github.com/BAMWelDX/weldx/pull/583/checks?check_run_id=3827087537#step:10:178
not sure why it sometimes keep popping up, should be unrelated here and my guess is some kind of wrong intersphinx loading

@vhirtham
Copy link
Collaborator Author

vhirtham commented Oct 7, 2021

did you do anything specific to fix this error? https://github.com/BAMWelDX/weldx/pull/583/checks?check_run_id=3827087537#step:10:178 not sure why it sometimes keep popping up, should be unrelated here and my guess is some kind of wrong intersphinx loading

Didn't try it so far, but busting those warnings "the right way" is extremely time-consuming. Try a fix and wait 5 minutes to see if it worked. If we want to get a warning-free build, I can just put it into the nitpick ignore. Is that okay?

@CagtayFabry
Copy link
Member

did you do anything specific to fix this error? https://github.com/BAMWelDX/weldx/pull/583/checks?check_run_id=3827087537#step:10:178 not sure why it sometimes keep popping up, should be unrelated here and my guess is some kind of wrong intersphinx loading

Didn't try it so far, but busting those warnings "the right way" is extremely time-consuming. Try a fix and wait 5 minutes to see if it worked. If we want to get a warning-free build, I can just put it into the nitpick ignore. Is that okay?

did you do anything specific to fix this error? https://github.com/BAMWelDX/weldx/pull/583/checks?check_run_id=3827087537#step:10:178 not sure why it sometimes keep popping up, should be unrelated here and my guess is some kind of wrong intersphinx loading

Didn't try it so far, but busting those warnings "the right way" is extremely time-consuming. Try a fix and wait 5 minutes to see if it worked. If we want to get a warning-free build, I can just put it into the nitpick ignore. Is that okay?

it's probably a good use for the nitpick ignore 👍

@vhirtham vhirtham merged commit 08faea5 into BAMWelDX:master Oct 8, 2021
@vhirtham vhirtham deleted the geometry_enhancements branch October 8, 2021 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geometry visualization plotting and related functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants