Check units of coordinates #683
Conversation
vhirtham
added
transformations
| # The first branch is a workaround until units are mandatory | ||
| if coordinates.data.u == pint.Unit(""): | ||
| coordinates.data = coordinates.data.m |
There was a problem hiding this comment.
This looks dangerous to me, I guess the assumption is that if a 'dimensionless' quantity is passed we interpret it as 'default unit' ?
I would prefer to be strict here and force all quantities to be [length] so that at least the quantity behavior is already consistent
There was a problem hiding this comment.
The problem is that our "unitless" code breaks at multiple places when we remove this line. This is just a workaround to enable units as input without enforcing them. For example, the "TimeSeries" returns unitless quantities in several tests and examples/notebooks. Fixing those problems is part of the transitions second step.
There was a problem hiding this comment.
ah I see, thank you
Is it only tests/notebooks or also inside the package?
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report
@@ Coverage Diff @@
## master #683 +/- ##
==========================================
- Coverage 96.06% 96.05% -0.01%
==========================================
Files 92 92
Lines 6350 6392 +42
==========================================
+ Hits 6100 6140 +40
- Misses 250 252 +2
Continue to review full report at Codecov.
|
|
This is ready now. Remember that it is just an intermediate step with the goal to add unit support to LCS and CSM without breaking code that does not provide units. So there are some workarounds involved that will disappear later. What doesn't work anymore are mixed cases. So if some of the LCS in a CSM have units while some others don't, you will get an error. This was the case in some of the notebooks so I had to adjust them. |
There was a problem hiding this comment.
There seem to be quite a few UnitStrippedWarning in some of the tutorials now, e.g. https://weldx--683.org.readthedocs.build/en/683/tutorials/welding_example_02_weaving.html#add-a-sine-wave-to-the-TCP-movement
I guess it is the same line causing each
|
Couldn't stick to the original plan (not touching k3d) due to the unit stripped warnings now being errors. However, got everything working now. Additionally, I moved the default units to |
| " groove_angle=Q_(50, \"deg\"),\n", | ||
| " root_face=Q_(1, \"mm\"),\n", | ||
| " root_gap=Q_(1, \"mm\"),\n", | ||
| " workpiece_thickness=\"0.5 cm\",\n", |
There was a problem hiding this comment.
Would it also have been possible to pass a tuple[Union[float, int], str], e.g. (1, "mm")? I would prefer such an option, if these quantities are actually the result of a computation.
There was a problem hiding this comment.
not sure at this specific point since I didn't touch the internals of the function. Just fixed an issue that popped up due to the function using the LCS. Anyways, all newer functions that support units should usually provide several format options for quantities. I think a tuple is also possible, but I have to check it.
Changes
Adds unit support for the LCS and CSM coordinates without requiring units.
Related Issues
#682 (do not close)
Checks
update manifest file