cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
16 stars 21 forks source link

remove unit is unclear? #385

Open kerimoyle opened 5 years ago

kerimoyle commented 5 years ago

The unit.cpp test file contains this code, but I don't really understand its operation (see comments):

TEST(Units, multipleUnitUsingStandardRef)
{
    libcellml::Units u;

    u.addUnit(libcellml::Units::StandardUnit::AMPERE, "micro");
    u.addUnit(libcellml::Units::StandardUnit::AMPERE, "milli");
    u.addUnit(libcellml::Units::StandardUnit::AMPERE, libcellml::Prefix::CENTI);
    u.addUnit(libcellml::Units::StandardUnit::AMPERE, libcellml::Prefix::MICRO);

// So at this stage our unit u has (micro)(milli)(centi)(micro)amps^4

    EXPECT_EQ(size_t(4), u.unitCount());

    u.removeUnit(libcellml::Units::StandardUnit::AMPERE);

// How/why do we want to remove a unit which doesn't exist by itself?  Does this mean if I had a unit
// of prefix=milli, unit=metres I could remove the metres and be left with a multiplier of a 
// non-//dimensional unit? Or, why not remove all of the amp references?  Why just the first one? It 
// seems strange to rely on the order in which the units were added to define what you're left with?

    EXPECT_EQ(size_t(3), u.unitCount());

    std::string prefix;
    std::string reference;
    std::string id;
    double exponent;
    double multiplier;

// Where there is more than one unit with amps in it, why do we allow a reference to call it? 
// Wouldn't it make more sense to return the set of units with that reference?

    u.unitAttributes(libcellml::Units::StandardUnit::AMPERE, prefix, exponent, multiplier, id);
    EXPECT_EQ("milli", prefix);
    u.unitAttributes(0, reference, prefix, exponent, multiplier, id);
    EXPECT_EQ("milli", prefix);
    u.unitAttributes(1, reference, prefix, exponent, multiplier, id);
    EXPECT_EQ("centi", prefix);
    u.unitAttributes(2, reference, prefix, exponent, multiplier, id);
    EXPECT_EQ("micro", prefix);
}
hsorby commented 5 years ago

It is not ours to wonder why a person may or may not want to remove a particular unit from a units we are aiming to give that person tools that will allow them to remove a unit from a units.

The documentation makes it clear that using the remove unit by reference will remove the first unit with that reference found. There is not much more that you can do.