Units rework II #18844

bofdahof · 2025-01-03T12:48:40Z

Continues on from the work of @3x380V
Make constexpr.
Make immutable.
Separate data from code.
Move units setup to namespace Units in Base/Units.h. Prefer algorithms to raw loops.
Cover more behaviors in unit tests.
Reduce repetition.

hyarion · 2025-01-03T22:18:50Z

There is a lot of good stuff in here but you are touching so much stuff at once that it's really difficult to review.

I know it's a lot to ask for but would it be possible to split it up or maybe skip the change from Unit to Units in this PR, as that seems to be most line changes?

bofdahof · 2025-01-04T05:34:06Z

Simple answer: Unfortunately, no. Responsibilities had to be untangled.

In terms of quantity of files touched the vast majority are simply changing 'Unit' to 'Units'. There are indeed plenty of them, but very simple.

In terms of files with lots of changes, there is just Unit which is much much simpler after replacing bitfields with int8_t array, and new Units after separating code and data.

Unit tests cover more behaviours to improve confidence.

3x380V · 2025-01-04T15:14:09Z

Well, at least renames and the rest deserve separate commits. If anything it is way easier to justify changes. I can do the work, but a bit later. Otherwise I'm very happy you are working on it.

kadet1090

Overall solid, but I'm really unsure about the Unit into Units change. First of all it introduces a lot of code changes, which would be fine if it would bring good benefits. I however cannot seem to see them, if anything I think that it is worse.

For example, Quantity(1.2340, Unit::Mass) reads quite well quantity 1.234 unit mass makes sense when read aloud. On the other hand Quantity(1.2340, Units::Mass) reads quantity 1.234 units mass and the plural units here are really odd to read.

This is absolutely nitpicking but it is hard for me to justify so much changes to code while the result is not obviously better.

kadet1090 · 2025-01-04T16:51:30Z

src/App/Expression.cpp

@@ -503,7 +504,7 @@ static inline Quantity pyToQuantity(const Py::Object &pyobj,
 }

 Py::Object pyFromQuantity(const Quantity &quantity) {
-    if(!quantity.getUnit().isEmpty())
+    if(quantity.getUnit() != Units::NullUnit)


Why is this better than previous way? I cannot see any good reason for that change, buy maybe I am missing something?

I'd also highly prefer NoUnit as name here as NullUnit is not real concept.

Isn't this usually called Unitless?

Yes, that seems even better.

Consider: Unit is the class for a single unit. How and where Unit instances are made or used is no business of Unit.

OTOH Units is a place where units are defined and created, including a large set of predefined units.

I'm not real happy about such a large number of predefined unit variables as it tends to codifying data, in the same way an enum would (bad), but left it alone for now pending a better solution. It is really more suited to be a single container of units -- which already exists now as the definitions. It crossed my mind to build units on demand using the name string. Being constexpr there is no performance hit and mistakes will be caught at compile time. But strings can be a pain too. For future thought.

When we say Units::NullUnit we are referring to the variable NullUnit from the place (Units) where units live. BTW they should all be renamed to (lower) camelCase so it is clear they are variables.

It is quite common to have a version of an object which does nothing. Commonly known as the Null Object. It is a valid object, completes the set of objects, can be tested against compared etc. Also means no code needed for the special case of what was before called empty unit. Part of separating code and data, and making orthogonal definitions in the data instead of introducing special cases later (a huge source of unwanted complexity).

Also important to remember Unit is now immutable, so there is no way to setup an empty Unit and then change it. There is a default constructor. I'd like it to be delete, but couldn't immediately get round a usage that exists. One for later.

I know the null object - it is a good thing to have one here! However, my comment was more about the API not about the fact that NullUnit exists.

First, it really should be renamed into Unitless so it is named more inline with the domain that it represents.
Second, isEmpty method could be kept as-is, it would work fine even with null object concept - simply only the null instance will return true. It will result with less code changes.

That said, renaming it to Unitless would be enough I think as naming it according to the domain would be a good thing as empty unit is not a thing either.

The domain here would be probably something like metrology. NullUnit does not really represent an unit, so by your own words it should really not exist. But we have it as technical placeholder to ease our life - this does not mean however that we need to give it very technical name, let more people understand it.

So please name it in a way that if I show this code to some engineer or physicist (which I would consider to have better understanding of unit algebra, and so be domain experts) they won't ask "And what is that 'Null' unit, what does it represent?" - this is all I am asking for.

by your own words it should really not exist

Sorry, but that's rubbish! It absolutely does and should exist to represent the perfectly valid Unit with all-zero vals, defined in precisely the same manner as all other predefined units.

But in real world there is no such unit as null unit.

This is getting silly! There are many things in programming that could be said to not exist in the real world!!

Unit models a thing from real world, so it is not perfectly valid unit if it does not represent a thing existing in real world. Unit with all exponents set to 0 represents in real life simply not having an unit - so the concept exists and maps quite well. So please name it in a way that if I show this code to some engineer or physicist (which I would consider to have better understanding of unit algebra, and so be domain experts) they won't ask "And what is that 'Null' unit, what does it represent?".

kadet1090 · 2025-01-04T16:53:22Z

src/App/Expression.cpp

+        unit = v1.getUnit().root(2);
        break;
    case CBRT:
-        unit = v1.getUnit().cbrt();
+        unit = v1.getUnit().root(3);


I'm not sure if this is a good change. sqrt is well known name for calculating square root. The root(2) on the other hand I see first time. For me root(n) seems more like function that could be applied to graphs, or something like that.

I'm assuming that this change is done to remove the redundant work for both sqrt and cbrt? If this is a concern, would it be possible to use pow(1/n) instead?

Yes, probably, but even if sqrt is nothing else than pow(1/n) the sqrt method is also often expected to exist. This is the most common root to call so such convenience method is useful. It of course can be implemented simply as inline Unit sqrt() { return root(2); } (or using pow(...)), but should not be removed.

Maybe we start a trend!

I'm assuming that this change is done to remove the redundant work for both sqrt and cbrt?

Yes, sqrt and cbrt were almost the same but duplicated with different tests. So, indeed, they got refactored to eliminate repetition. Exactly the same code can handle both, without if/else.

Existing callers of pow(n) inherently have an expression as the argument so pow(n) is consistent with that. Maybe that's where I got the idea.

root is only accessed twice so it didn't seem like a concern to go for the more elegant solution consistent with pow, leaving the Unit api as simple as possible?

According to the (updated) test pow_less_then_one, calling pow with an argument less than one is valid and expected to work. If there's a case where it doesn't work then that means you've found a bug.
In other words, the root(n) function can be removed in favor for pow(1/n) to avoid multiple implementations of the same functionallity.

Firstly:

TEST(Unit, TestPowLT1) { Base::Unit unit {3}; EXPECT_EQ(unit.pow(1.0 / 3.0), Base::Unit {1}); }

was refactored to:

TEST(Unit, pow_less_then_one) { EXPECT_EQ(Volume.pow(1.0 / 3.0), Length); }

so test behavior unchanged.

I'm uncomfortable on any change to this as the original code has two very different implementations. I'm not disagreeing what you say could be done. My question is should it be done.

I think I prefer to defer to core maintainer @wwmayer on this one.

From math point of view n-th root is the same thing as 1/n-th power. Now lets look at the code, for root you have

auto apply = [&](auto val) { return static_cast<int8_t>(val / num); };

For pow you have

auto apply = [&](const auto val) { return static_cast<int>(val * exp); };

So by definition it is the same as division by n is multiplication by 1/n, you don't need any conditional logic. The rest of the code is eerily similar - not to say identical and duplicated.

looking more closely you also introduce something that is very hard to udnerstand. Your method isInt(double val) does not check if val is int what would be expected from the method name. It checks if val * exp is int which is not contained in the name and hence is very easy to miss.

In the revisions I have on standby the isInt is folded into apply.

Ok, so until it is fixed I'll add comments on that problem so it would be easier to track.

kadet1090 · 2025-01-04T16:59:33Z

src/Gui/propertyeditor/PropertyItem.cpp

@@ -1836,7 +1836,7 @@ QVariant PropertyVectorDistanceItem::editorData(QWidget* editor) const

 Base::Quantity PropertyVectorDistanceItem::x() const
 {
-    return Base::Quantity(data(1, Qt::EditRole).value<Base::Vector3d>().x, Base::Unit::Length);
+    return Base::Quantity(data(1,Qt::EditRole).value<Base::Vector3d>().x, Base::Units::Length);


Nitpick, but, why the space after 1, got removed?

No idea. But reformat the file fixed it, and some others around line 2825

kadet1090 · 2025-01-04T17:01:30Z

src/Mod/Sketcher/Gui/TaskSketcherConstraints.cpp

+    QIcon icon_## FUNC(Gui::BitmapFactory().pixmap(ICONSTR));                                       \
+    QAction* constr_## FUNC = menu.addAction(icon_## FUNC, tr(NAMESTR), this, SLOT(FUNC()));         \
+    constr_## FUNC->setShortcut(QKeySequence(QString::fromUtf8(                                     \
        Gui::Application::Instance->commandManager().getCommandByName(CMDSTR)->getAccel())));      \
    if (ACTSONSELECTION)                                                                           \
-        constr_##FUNC->setEnabled(!items.isEmpty());                                               \
+        constr_## FUNC->setEnabled(!items.isEmpty());                                               \
    else                                                                                           \
-        constr_##FUNC->setEnabled(true);
+        constr_## FUNC->setEnabled(true);


Is this even legal to put additional space here? Even if this does not seem good to do so as it should be read as one.

\ are misaligned

I did have some strange difficulties with files I wasn't working on becoming involved. My IDE auto set line endings (which I turned off) and also stray spaces at the end of a line. So just opening a file sometimes meant the file had changed and finished up in the commit. On top of that the precommit bot jumped in and made some changes.
So, yeah, some snuck through.

Begs the bigger question: What happened to the push to clang-format everything? These difficulties, which seem to pop up regularly, would disappear, I think?

Is this even legal

Don't know, but would tend to think not. Caused by some mystery clang-format. Will fix

kadet1090 · 2025-01-04T17:01:47Z

src/Mod/Sketcher/Gui/TaskSketcherConstraints.cpp

@@ -280,7 +280,7 @@ class ConstraintItem: public QListWidgetItem
                    return normal;
                }
                else {
-                    return driven;
+                return driven;


Indentation is broken here.

kadet1090 · 2025-01-04T17:01:52Z

src/Mod/Sketcher/Gui/TaskSketcherConstraints.cpp

@@ -348,7 +348,7 @@ class ConstraintItem: public QListWidgetItem
                return QVariant();
        }
        else
-            return QListWidgetItem::data(role);
+        return QListWidgetItem::data(role);


Indentation is broken here.

kadet1090

While this introduces some improvements and cleanups, it is not yet in state to be merged. Constructive feedback with suggestions on how to improve the PR was provided and awaits to be addressed.

kadet1090 · 2025-01-05T23:53:01Z

src/Base/Unit.cpp

+    auto isInt = [&exp](const auto val) {
+        const auto num {val * exp};
+        return std::fabs(std::round(num) - num) < std::numeric_limits<double>::epsilon();
+    };


Name of the function is not really suited as it does not check if val is int, but some arbitrary expression inside the function. This is very easy to miss and thus has great potential for being misleading.

kadet1090 · 2025-01-05T23:54:47Z

src/Base/Unit.cpp

+Unit Unit::root(const uint8_t num) const
 {
-    UnitSignature sig;
-    memcpy(&sig, &Val, sizeof(Val));
-    return sig.LuminousIntensity;
-}
+    auto check = [&](auto val) {
+        return val % num == 0;
+    };

-int Unit::angle() const
-{
-    UnitSignature sig;
-    memcpy(&sig, &Val, sizeof(Val));
-    return sig.Angle;
-}
+    auto apply = [&](auto val) {
+        return static_cast<int8_t>(val / num);
+    };

-bool Unit::isEmpty() const
-{
-    return Val == 0;
-}
+    //---------------------------------------------------------

-int Unit::operator [](int index) const
-{
-    UnitSignature sig;
-    memcpy(&sig, &Val, sizeof(Val));
-
-    switch (index) {
-       case 0:
-          return sig.Length;
-       case 1:
-          return sig.Mass;
-       case 2:
-          return sig.Time;
-       case 3:
-          return sig.ElectricCurrent;
-       case 4:
-          return sig.ThermodynamicTemperature;
-       case 5:
-          return sig.AmountOfSubstance;
-       case 6:
-          return sig.LuminousIntensity;
-       case 7:
-          return sig.Angle;
-       default:
-          throw Base::IndexError("Unknown Unit element");
+    if (num < 1) {
+        throw UnitsMismatchError("root must be > 0");
    }
-}

-bool Unit::operator ==(const Unit& that) const
-{
-    return Val == that.Val;
+    if (!std::all_of(vals.begin(), vals.end(), check)) {
+        throw UnitsMismatchError("unit values must be divisible by root");
+    }
+
+    std::array<int8_t, unitNumVals> res {};
+    std::transform(vals.begin(), vals.end(), res.begin(), apply);
+
+    return Unit {res};


This method should be equivalent to calling pow(1 / n) and looking at code it really is. Check here is implemented in another way but in the end it checks the same thing - if the result of operation is integer. This method should be removed and calls like root(n) should be replaced with pow(1/n). Addressing that would also address my concerns about sqrt and cbrt functions.

kadet1090 · 2025-01-05T23:55:28Z

src/Base/Unit.cpp

+    auto checkPow = [&] {
+        if (!std::all_of(vals.begin(), vals.end(), isInt)) {
+            throw UnitsMismatchError("pow() of unit not possible");
        }
+    };


Does it need to be lambda? It is called only once, the call is redundant.

kadet1090 · 2025-01-06T00:01:56Z

src/Base/Unit.cpp

-            if (sig.Angle < -1) {
-                ret << "^" << abs(sig.Angle);
-            }
+    auto posNeg = [&](auto index) {


Please give it more describing name like splitIntoPosOrNeg

maxwxyz · 2025-01-10T15:19:32Z

waiting on feedback

bofdahof · 2025-01-10T15:21:50Z

This PR has been nitpicked to death

Make constexpr. Make immutable. Separate data from code. Move units setup to namespace Units in Base/Units.h. Prefer algorithms to raw loops. Cover more behaviors in unit tests. Eliminate repetition.

maxwxyz requested a review from wwmayer January 3, 2025 13:04

kadet1090 reviewed Jan 4, 2025

View reviewed changes

bofdahof force-pushed the units branch from ce7580d to 3669b3e Compare January 4, 2025 21:00

kadet1090 suggested changes Jan 6, 2025

View reviewed changes

bofdahof mentioned this pull request Jan 10, 2025

Move predefined units to namespace Units #18976

Closed

bofdahof marked this pull request as draft January 10, 2025 15:22

bofdahof force-pushed the units branch 5 times, most recently from fd3a035 to 802a42f Compare January 16, 2025 04:25

Simplify Unit

5ff9afd

Make constexpr. Make immutable. Separate data from code. Move units setup to namespace Units in Base/Units.h. Prefer algorithms to raw loops. Cover more behaviors in unit tests. Eliminate repetition.

bofdahof force-pushed the units branch from 802a42f to 5ff9afd Compare January 17, 2025 22:36

Units rework II #18844

Are you sure you want to change the base?

Units rework II #18844

Conversation

bofdahof commented Jan 3, 2025

hyarion commented Jan 3, 2025

bofdahof commented Jan 4, 2025

3x380V commented Jan 4, 2025

kadet1090 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bofdahof Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyarion Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadet1090 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxwxyz commented Jan 10, 2025

bofdahof commented Jan 10, 2025

bofdahof Jan 4, 2025 •

edited

Loading

hyarion Jan 5, 2025 •

edited

Loading