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

Strict quantities #731

Merged
merged 49 commits into from
Apr 28, 2022
Merged

Strict quantities #731

merged 49 commits into from
Apr 28, 2022

Conversation

vhirtham
Copy link
Collaborator

@vhirtham vhirtham commented Mar 31, 2022

Changes

Units are now mandatory and no longer optional in the CoordinateSystemManager and the LocalCoordinateSystem

Related Issues

Closes #682

Checks

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

@github-actions
Copy link

github-actions bot commented Mar 31, 2022

Unit Test Results

       1 files  ±  0         1 suites  ±0   2m 34s ⏱️ -2s
2 146 tests  - 17  2 146 ✔️  - 17  0 💤 ±0  0 ±0 

Results for commit 0aad9d2. ± Comparison against base commit 63d1ce6.

♻️ This comment has been updated with latest results.

@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 Mar 31, 2022

Codecov Report

Merging #731 (0aad9d2) into master (63d1ce6) will decrease coverage by 0.04%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
- Coverage   95.89%   95.85%   -0.05%     
==========================================
  Files          92       92              
  Lines        6511     6520       +9     
==========================================
+ Hits         6244     6250       +6     
- Misses        267      270       +3     
Impacted Files Coverage Δ
weldx/time.py 97.76% <ø> (ø)
weldx/geometry.py 96.59% <77.77%> (-0.21%) ⬇️
weldx/transformations/local_cs.py 96.70% <87.50%> (-0.43%) ⬇️
weldx/asdf/cli/welding_schema.py 97.02% <100.00%> (ø)
weldx/tags/core/geometry/spatial_data.py 100.00% <100.00%> (ø)
...gs/core/transformations/local_coordinate_system.py 100.00% <100.00%> (ø)

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 63d1ce6...0aad9d2. Read the comment docs.

@vhirtham vhirtham marked this pull request as ready for review April 4, 2022 13:56
@vhirtham
Copy link
Collaborator Author

I think the only things missing are that we need new schema file versions for LCS and CSM with that added unit restriction via wx_units

Only the LCS needed an update, since the CSMs unit restrictions are just a result of the stored LCS instances. I updated the LCS schema version.

By the way, should we have also created a new version for the SpatialData schema?

@marscher
Copy link
Contributor

Thanks you for this improvement.
At the very end I suggest adding a test, which checks the raising of the exceptions, if no unit was provided.

@CagtayFabry CagtayFabry added this to the 0.6.0 milestone Apr 28, 2022
@vhirtham vhirtham requested a review from CagtayFabry April 28, 2022 10:22
weldx/geometry.py Outdated Show resolved Hide resolved
@CagtayFabry CagtayFabry self-requested a review April 28, 2022 13:42
@CagtayFabry
Copy link
Member

CagtayFabry commented Apr 28, 2022

the only problem left is probably opening old weldx files now since that would require adding the default quantities in the from_yaml conversion

@vhirtham please add a list to the PR comment indicating which classes are affected by this PR
should be fine now

@CagtayFabry CagtayFabry merged commit a1e7184 into BAMWelDX:master Apr 28, 2022
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.

CSM/LCS unit support
3 participants