cellml
Model::removeUnits / findUnits behaves differently between name and reference arguments
This bug is coming from the differences in how we search for units in a model using the findUnits function (see below). I can understand searching for a name match (since this is how we do everything else), but I don't understand the reference search. What's the rationale for not simply testing that the pointer is found?? Why test name and definition?
If we do have a need to test the name and definition for units, can we move that to a separate explicit function and have the default behaviour in line with the other classes? @agarny @hsorby @nickerso what do you think?
std::vector<UnitsPtr>::iterator Model::ModelImpl::findUnits(const std::string &name)
{
return std::find_if(mUnits.begin(), mUnits.end(),
[=](const UnitsPtr &u) -> bool { return u->name() == name; });
}
std::vector<UnitsPtr>::iterator Model::ModelImpl::findUnits(const UnitsPtr &units)
{
return std::find_if(mUnits.begin(), mUnits.end(),
[=](const UnitsPtr &u) -> bool { return units->name().empty() ? false : u->name() == units->name() && Units::equivalent(u, units); });
}
Here's the test that fails:
auto model = libcellml::Model::create();
auto myImportedComponent = libcellml::Component::create("myImportedComponent");
auto myConcreteComponent = libcellml::Component::create("myConcreteComponent");
auto myConcreteUnits = libcellml::Units::create("myConcreteUnits");
auto myImportedUnits = libcellml::Units::create("myImportedUnits");
auto import = libcellml::ImportSource::create();
import->setUrl("import.cellml");
myImportedComponent->setImportSource(import);
myImportedUnits->setImportSource(import);
model->addComponent(myImportedComponent);
model->addComponent(myConcreteComponent);
model->addUnits(myConcreteUnits);
model->addUnits(myImportedUnits);
auto success = model->removeUnits(myImportedUnits);
EXPECT_TRUE(success); // <<<<<<<<<< fails, units are not found!
success = model->removeUnits("myImportedUnits");
EXPECT_TRUE(success); // << passes.
The reason that the find is not working is because the units are imported, so we can't test for equivalence. That's fine ... except that the pointers are identical, which surely should take precedence over names and definitions! I suggest that if we want to keep searching by name and contents we either:
- search FIRST by pointer; search SECOND by name; search FINALLY by definition and equivalence; or
- explicitly separate the search by definition into another function name so that the default is by pointer.
I believe I commented on this at some point (in some GitHub issue somewhere) and the fact that depending on how you are trying to find a units, you may get different results... as you are showing here. Personally, it's simple, I still don't like it.
I'm going to make a quick PR that adds a check by pointer first when a reference is passed. We can discuss it there once we know what else breaks ...!
I think this is really a duplication of #719 and that this issue should be closed.
