Remove unit checking from the backend

[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.

[perost]

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

[casella]

Replying to perost:

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

Of course, thanks!

[phannebohm]

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?

[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.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.

[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 :)

[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.

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.

[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?

[phannebohm]

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 :)

[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.

[phannebohm]

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.

[casella]

@perost, could you please have a look?

[perost]

unit should be var on ​this line I guess?

[phannebohm]

Replying to perost:

unit should be var on ​this line I guess?

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

[phannebohm]

Fixed in ​PR6771

[casella]

Thanks Per