Fix unit error in IdealGasH2O

[henrikt-ma]

The change is prepared following the same style as in the more general #4116.

[HansOlsson]

I'm not sure if this is needed - and would discuss it together with #4097

[HansOlsson]

Similar considerations here.

[henrikt-ma]

Similar considerations here.

As there aren't too many errors like this in the MSL, I don't see why we can't just fix them.

[HansOlsson]

Similarly as #4103 and we should do these consistently

• The current model is a somewhat sloppy example that is badly documented; how forgiving we should be is unclear. Here we are documenting and parametrization the original code instead of rewriting it; just making it harder to understand and having lots of parameters that don't matter and don't have good documentation (a comment such as "state.p rate of change" is a code smell for me); I would prefer that we keep it "as is" if we can't do it right.
• There is documentation of the model, but it is incorrect as there is no "medium2" (I assume the isentropic part is correct)). My interpretation is that smoothly switches smoothState from state to state2; with similar plot as in Fix unit error in SimpleLiquidWater model #4103
• We are doing two things: smoothly switching between two states, while one of them is "statically" sweeping from one temperature to another (in contrast to the next example); so the important part are the two temperatures (start and end) and time for sweep; not the change per second. We might also skip all of the non-changed variables - if nothing changes for state2 there's no need to have parameters for it. They should be parameters (likely public).

[henrikt-ma]

• We are doing two things: smoothly switching between two states, while one of them is "statically" sweeping from one temperature to another (in contrast to the next example); so the important part are the two temperatures (start and end) and time for sweep; not the change per second. We might also skip all of the non-changed variables - if nothing changes for state2 there's no need to have parameters for it. They should be parameters (likely public).

I'd like to give this and the similar PRs another try without waiting for the language to introduce syntax for attaching units to literals (which would have made it trivial to fix these unit errors). I agree that having similar discussions in many PRs at the same time isn't good, so I'll try to get this one merged before turning to the others.

I'm ready to reformulate with a smaller set of (public) parameters for describing the transition, but how do you suggest to handle the sweep's relation to StartTime and StopTime? Maybe it's time to inject some new momentum into the discussion here about an evaluable expression for StopTime?

[hubertus65]

This ticket and the related tickets were discussed in the MAP-LIB meeting on 2023-11-14. There has been a voting with 6 in favor and 3 abstain to accept the proposed changes by Henrik. Hubertus mentioned that is fixes are in line with the model intent, and make them compatible with unit checking and the current MLS w.r.t. unit checking. The same situation and vote holds for tickets 4103,4116,4115,4114 ,4119, and 4112, which consequently can be merged. @HansOlsson: could you please either retract your change request and merge these tickets, or request another reviewer?

[HansOlsson]

This ticket and the related tickets were discussed in the MAP-LIB meeting on 2023-11-14. There has been a voting with 6 in favor and 3 abstain to accept the proposed changes by Henrik. Hubertus mentioned that is fixes are in line with the model intent, and make them compatible with unit checking and the current MLS w.r.t. unit checking. The same situation and vote holds for tickets 4103,4116,4115,4114 ,4119, and 4112, which consequently can be merged. @HansOlsson: could you please either retract your change request and merge these tickets, or request another reviewer?

Can't we do the previously suggested idea of specifying two states instead and switch between them instead?
Something like:

   parameter SI.AbsolutePressure p_start = 100000.0 "state.p at start";
   parameter SI.AbsolutePressure p_end = p_start "state.p at end";
   parameter SI.Temperature T_start = 200 "state.T at start";
   parameter SI.Temperature T_end = 1200 "state.T at end";
   parameter SI.AbsolutePressure p2_start = 2.0e5 "state2.p at start";
   parameter SI.AbsolutePressure p2_end = p2_start "state2.p at end";
   parameter SI.Temperature T2_start = 500 "state2.T at start";
   parameter SI.Temperature T2_end = T2_start "state2.T at end";
   constant SI.Time endTime=1;
 equation
   state.p = p_start+(p_end-p_start)*time/endTime;
   state.T = T_start+(T_end-T_start)*time/endTime;
   state2.p = p2_start+(p2_end-p2_start)*time/endTime;
   state2.T = T2_start+(T2_end-T2_start)*time/endTime;

Note that T2_end, p2_end and p_end aren't needed, and could be skipped (and endTime could be made into a parameter, ofc).

Removing them makes the model simpler:

   parameter SI.AbsolutePressure p = 100000.0 "state.p";
   parameter SI.Temperature T_start = 200 "state.T at start";
   parameter SI.Temperature T_end = 1200 "state.T at end";
   parameter SI.AbsolutePressure p2 = 2.0e5 "state2.p";
   parameter SI.Temperature T2 = 500 "state2.T";
   constant SI.Time endTime=1;
 equation
   state.p = p;
   state.T = T_start+(T_end-T_start)*time/endTime;
   state2.p = p2;
   state2.T = T2;

[henrikt-ma]

Considering MAP-Lib monthly's (yesterday) desire to move on with PRs about fixing unit issues, I suggest that we open separate PRs about improving example parameterizations once the unit-related PRs have been merged.

[HansOlsson]

Considering MAP-Lib monthly's (yesterday) desire to move on with PRs about fixing unit issues, I suggest that we open separate PRs about improving example parameterizations once the unit-related PRs have been merged.

Will do, except I think it should be one PR for the similar media-models (that require updates).

One of the issues with these PRs are that by being different PRs they use different stylistic choices (naming and visibility: constant, protected parameter, public parameter) due to being spread over multiple PRs, and similarly the discussion have been spread out.

The same situation and vote holds for tickets 4103,4116,4115,4114 ,4119, and 4112, which consequently can be merged. @HansOlsson: could you please either retract your change request and merge these tickets, or request another reviewer?

Done (except #4119 - where I'm not blocking) .
Note that for many of them I hadn't requested changes, just left comments - including the part above.

[HansOlsson]

Ok, according to MAP-Lib. Will update later

[hubertus65]

Ok as is now.