ConfigurationError for ControlVolumes regarding time units

[adam-a-a]

It appears that the following configuration error in ControlVolume0D (and similarly for 1D) will not be called when time_units is set to None while instantiating a Flowsheet:

if self.config.dynamic:
    f_time_units = self.flowsheet().time_units
    if (f_time_units is None) ^ (units("time") is None):
        raise ConfigurationError(
            "{} incompatible time unit specification between "
            "flowsheet and property package. Either both must use "
            "units, or neither.".format(self.name)
        )

Instead, the ConfigurationError from FlowsheetBlockData will be raised:

elif self.config.time_units is None and self.config.dynamic:
    raise ConfigurationError(
        f"{self.name} - no units were specified for the time domain. "
        f"Units must be be specified for dynamic models."

So it seems the first ConfigurationError that I mentioned will only be raised if time_units are set appropriately and no units are specified for the property model. Notably, there seems to be no testing to ensure the config error occurs.

Additionally, there is no safeguard in the case where the user might provide dimensionless units for the flowsheet and units for the property model. For example, setting time_units to dimensionless for the Flowsheet while using units in the property package will result in a InconsistentUnitsError for material_balances: InconsistentUnitsError: Error in convert: units not compatible.: mole not compatible with mole / second.

[adam-a-a]

For clarity, this is something that I can fix, but it is not at the top of my list of things to do yet. So I am posting this issue here for awareness and in case someone else has time to address it before I can (or in case the team votes that it is low enough priority not to pursue).

[ksbeattie]

Moving this to Nov. @adam-a-a is this something you think you'll get to by then?

[ksbeattie]

@adam-a-a, says this is mostly just a notice/reminder for the idaes dev team. It's a low priority for him, if someone on the idaes teams wants to tackle this go for it, otherwise he won't get to this by Nov.

[andrewlee94]

@adam-a-a Is this actually an issue/error? This actually looks more like legacy code to cover for when we were transitioning to units. For dynamic models, the flowsheet now insist that the time domain be given units, and thus you should never have the case of the flowhseet time domain being unitless. The other possible cause for this would be a property package not assigning units for time, but I think that is also not supported any more making it difficult or impossible to trigger this error.