Remove unit checking from the backend

[Francesco Casella]

The unit checking feature was first implemented as a backend module. It was later decided that it was best to apply it before any symbolic manipulation takes place, to make sure that no relevant information is lost, so the functionality was ported to the old frontend, and then ported from there to the new frontend.

The old frontend is going to be discontinued soon, so that is no big deal. Since the old backend is still going to stick around for some time, it would be best to get rid of it there.

@lochel, could you please remove the unitChecking --preOptModules compiler option right away, so this is fixed already for 1.16.0? This should take no time. I'm not sure how much effort it is to remove a module which is no longer used without breaking the compiler, if you could also do it, that would be great.

[Per Östlund]

Cc:ing lochel, @username doesn't work here.

[Francesco Casella]

Replying to perost:

Cc:ing lochel, @username doesn't work here.

Of course, thanks!

[Philip Hannebohm]

I'm working on it. I'll try to change it so the compiler option activates whatever is there in the Frontend.

Should I also give a Notification that this option is deprecated? Is there already some standard way to handle dead compiler options?

[Francesco Casella]

Replying to phannebohm:

I'm working on it. I'll try to change it so the compiler option activates whatever is there in the Frontend.

Hmmm, I would find it weird that a -preOptModules flag influences the frontend. But it's ok if you add a warning (not a notification) and some text like "this module is no longer available and its use is deprecated. XXX was used instead".

Should I also give a Notification that this option is deprecated? Is there already some standard way to handle dead compiler options?

In fact, there isn't even a proper way to handle wrong compiler options in the first place, see #5724.

I think we should issue a warning that they are deprecated. I'm not sure if we want to keep them in the documentation, maybe we can do that for a while and remove them wholesale in 2.0.0?

[Karim Adbdelhak]

Replying to casella:

Hmmm, I would find it weird that a -preOptModules flag influences the backend.

I guess you mean the frontend? Because it always affects the backend.

[Francesco Casella]

Replying to Karim.Abdelhak:

Replying to casella:

Hmmm, I would find it weird that a -preOptModules flag influences the backend.

I guess you mean the frontend? Because it always affects the backend.

Of course, I just corrected my comment :)

[Philip Hannebohm]

Okay after lying in bed for a few weeks I'll try to finish this quickly.

I found an old flag --unitChecking. The code that gets activated by this flag lives in the old frontend and doesn't seem to work correctly, so I commented it out of meaningful existence. (Maybe we can delete it but I didn't bother since the old frontend is supposed to vanish soon anyways and it looks like too much effort to remove all parts. Also I'm not sure what parts of it may still be of interest for integrating into the newer stuff.)

Now that --unitChecking is free for use, I made -d=frontendUnitCheck, -d=oldFrontendUnitCheck, --preOptModules=unitChecking and --preOptModules+=unitChecking (why is that a separate thing?!?) refer to --unitChecking. So the former ones are now deprecated and the latter is the supported one.

There are three (not so) different folders for unit checking in the testsuite. I will merge them into a single one, I think that's the most reasonable thing to do, and also the last for this ticket. :)

It shouldn't take long, I'm not sure if it will be fast enough for ​Adrian's beta PR though.

[Francesco Casella]

Replying to phannebohm:

Okay after lying in bed for a few weeks I'll try to finish this quickly.

I found an old flag --unitChecking. The code that gets activated by this flag lives in the old frontend and doesn't seem to work correctly, so I commented it out of meaningful existence. (Maybe we can delete it but I didn't bother since the old frontend is supposed to vanish soon anyways and it looks like too much effort to remove all parts. Also I'm not sure what parts of it may still be of interest for integrating into the newer stuff.)

Now that --unitChecking is free for use, I made -d=frontendUnitCheck, -d=oldFrontendUnitCheck, --preOptModules=unitChecking and --preOptModules+=unitChecking (why is that a separate thing?!?) refer to --unitChecking. So the former ones are now deprecated and the latter is the supported one.

Which corresponds to the new front-end checking, right?

[Philip Hannebohm]

Replying to casella:

Which corresponds to the new front-end checking, right?

It corresponds to whichever frontend is active, old or new.
The backend module is gone.

EDIT: I just checked my changes, it should be 100% identical to what previously happened using -d=frontendUnitCheck, so yes new frontend checking :)

[Francesco Casella]

Replying to phannebohm:

EDIT: I just checked my changes, it should be 100% identical to what previously happened using -d=frontendUnitCheck, so yes new frontend checking :)

Sounds good, please go ahead. Maybe @adrpo can cheery pick this for 1.16.0 as well.

[Philip Hannebohm]

I'm nearly done. But I found a failing test (testsuite/simulation/modelica/NFunitcheck/UnitCheck18.mos). I don't know if this is a known bug, but when I activate the new frontend I get the following diff:

Equation mismatch: diff says:
--- /tmp/omc-rtest-phili/simulation/modelica/NFunitcheck/UnitCheck18.mos_temp3583/equations-expected	2020-10-22 14:05:48.539240552 +0200
+++ /tmp/omc-rtest-phili/simulation/modelica/NFunitcheck/UnitCheck18.mos_temp3583/equations-got	2020-10-22 14:05:50.135252899 +0200
@@ -33,19 +33,19 @@
 end unitCheckTests.UnitCheck18;
 "
 "[<interactive>:20:5-20:26:writable] Warning: The following equation is INCONSISTENT due to specified unit information: (y1, y2) = unitCheckTests.UnitCheck18.test(t1, t2)
 Warning: The units of following sub-expressions need to be equal:
 - sub-expression \"y1\" has unit \"m/s\"
-- sub-expression \"unitCheckTests.UnitCheck18.test().z\" has unit \"m\"
+- sub-expression \"unitCheckTests.UnitCheck18.test().m\" has unit \"m\"
 [<interactive>:20:5-20:26:writable] Warning: The following equation is INCONSISTENT due to specified unit information: (y1, y2) = unitCheckTests.UnitCheck18.test(t1, t2)
 Warning: The units of following sub-expressions need to be equal:
 - sub-expression \"y2\" has unit \"J\"
-- sub-expression \"unitCheckTests.UnitCheck18.test().r\" has unit \"m/s\"
+- sub-expression \"unitCheckTests.UnitCheck18.test().m/s\" has unit \"m/s\"
 [<interactive>:20:5-20:26:writable] Warning: The following equation is INCONSISTENT due to specified unit information: (y1, y2) = unitCheckTests.UnitCheck18.test(t1, t2)
 Warning: The units of following sub-expressions need to be equal:
 - sub-expression \"t2\" has unit \"m/s\"
-- sub-expression \"unitCheckTests.UnitCheck18.test().y\" has unit \"m\"
+- sub-expression \"unitCheckTests.UnitCheck18.test().m\" has unit \"m\"
 [<interactive>:21:5-21:18:writable] Warning: The following equation is INCONSISTENT due to specified unit information: b = unitCheckTests.UnitCheck18.test2(t2)
 Warning: The units of following sub-expressions need to be equal:
 - sub-expression \"t2\" has unit \"m/s\"
-- sub-expression \"unitCheckTests.UnitCheck18.test2().a\" has unit \"s\"
+- sub-expression \"unitCheckTests.UnitCheck18.test2().s\" has unit \"s\"
 "

It appears that input and output variables in functions are printed wrong, i.e. the unit is printed instead of the cref name. I don't see an easy fix for this, I don't even know what's happening here. The old frontend seems to work fine here.

[Francesco Casella]

@perost, could you please have a look?

[Per Östlund]

unit should be var on ​this line I guess?

[Philip Hannebohm]

Replying to perost:

unit should be var on ​this line I guess?

Yes I think that's it. (Crazy fast)

[Philip Hannebohm]

Fixed in ​PR6771

[Francesco Casella]

Thanks Per