Variable::clone() duplicates units without checking whether they exist in cloned model

[kerimoyle]

The series of cloning methods used inside Model::flatten() result in a cloned component with more units references than the original. This is because:

• the Variable::clone() automatically creates a new Units instance whether or not one exists:

VariablePtr Variable::clone() const
{
    auto v = create();
    if (mPimpl->mUnits != nullptr) {
        v->setUnits(mPimpl->mUnits->clone());   // <<<<<< no checking, just create a new Units item
    }
    v->setInitialValue(initialValue());
    v->setInterfaceType(interfaceType());
    v->setId(id());
    v->setName(name());
    return v;
}

• ... which in turn means that the usedUnits function collects more than one copy of the units within the flattenComponent method,
• ... which creates duplicated units where they should be the same (see test in Missing import units #520 (comment)).

At the model level this is fixed by calling the linkUnits() function, but since the flattening happens at a component level it's causing problems for the stuff happening within the flattenComponent() function. Any thoughts @agarny @hsorby @nickerso ?

[kerimoyle]

The true location of the unexpected behaviour is deep inside the dimensionallyEquivalent function, because one of the units being compared within the loop below has a null parent model, so the first hasUnits function returns false when it shouldn't:

flattenComponent() 
... go to end of function ... 

        // Copy over units used in imported component to this model.
        for (const auto &u : requiredUnits) {
            if (!model->hasUnits(u)) { // <<<<<<<<< this returns false when it shouldn't ...
                size_t count = 0;
                while (!model->hasUnits(u) && model->hasUnits(u->name())) {
                    auto name = u->name();
                    name += "_" + convertToString(++count);
                    u->setName(name);
                }
                model->addUnits(u);
            }
        }

[kerimoyle]

ie: calling model->hasUnits(u) where u has no parent model will always return false.

[hsorby]

Yes it does so the fix is to make sure the referenced units are findable. I have a solution for this. It is not the fault of variable clone method.

[hsorby]

ie: calling model->hasUnits(u) where u has no parent model will always return false.

This isn't entirely true, although it has led me to find an issue with equivalent units.

[kerimoyle]

You're right, it's actually the level below ... if u has unit children which are not defined in its model then hasUnits returns false.

[kerimoyle]

Ah, I think I've got it now. We don't take account of child units when creating the requiredUnits list - only those directly called by the variables. That means we end up with orphans later, so they can't be compared.
There are lot of issues with the units stuff - a lot of small changes are addressed in #554 Have you got others too?

[hsorby]

I have been making changes all over the place right now trying to put it back together so you can see what I hvae done.