Skip to content

Commit

Permalink
Stop registering FactorDefs to the UnitTable (#1030)
Browse files Browse the repository at this point in the history 
* Stop parsing of FactorDefs by the UnitParser that registered them in the UnitTable to follow the same policy as NEURON

* Throw error when trying to register unit that already exists in the UnitTable

* Added tests for UNITS redefinition and correct registration of UnitDefs and FactorDefs

* Use :g for printing factors of units in unit tests
  • Loading branch information
@iomaganaris
iomaganaris authored Apr 19, 2023
1 parent 3b148fc commit 5f864dc
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 131 deletions.
7 changes: 6 additions & 1 deletion src/parser/unit.yy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,12 @@ table_insertion
$$ = driver.table;
}
| table_insertion item {
$1->insert($2);
try {
$1->insert($2);
}
catch (std::runtime_error e) {
error(scanner.loc, e.what());
}
$$ = $1;
}
| table_insertion prefix {
Expand Down
33 changes: 16 additions & 17 deletions src/units/units.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,20 @@ void UnitTable::calc_denominator_dims(const std::shared_ptr<Unit>& unit,
}

void UnitTable::insert(const std::shared_ptr<Unit>& unit) {
// if the unit is already in the table throw error because
// redefinition of a unit is not allowed
if (table.find(unit->get_name()) != table.end()) {
std::stringstream ss_unit_string;
ss_unit_string << fmt::format("{:g} {}",
unit->get_factor(),
fmt::join(unit->get_nominator_unit(), ""));
if (!unit->get_denominator_unit().empty()) {
ss_unit_string << fmt::format("/{}", fmt::join(unit->get_denominator_unit(), ""));
}
throw std::runtime_error(fmt::format("Redefinition of units ({}) to {} is not allowed.",
unit->get_name(),
ss_unit_string.str()));
}
// check if the unit is a base unit and
// then add it to the base units vector
auto unit_nominator = unit->get_nominator_unit();
Expand All @@ -273,14 +287,7 @@ void UnitTable::insert(const std::shared_ptr<Unit>& unit) {
assert(index >= 0 && index < base_units_names.size());
// NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index)
base_units_names[index] = unit->get_name();
// if unit is found in table replace it
auto find_unit_name = table.find(unit->get_name());
if (find_unit_name == table.end()) {
table.insert({unit->get_name(), unit});
} else {
table.erase(unit->get_name());
table.insert({unit->get_name(), unit});
}
table.insert({unit->get_name(), unit});
return;
}
// calculate unit's dimensions based on its nominator and denominator
Expand All @@ -290,15 +297,7 @@ void UnitTable::insert(const std::shared_ptr<Unit>& unit) {
for (const auto& it: unit->get_denominator_unit()) {
calc_denominator_dims(unit, it);
}
// if unit is not in the table simply insert it, else replace with it with
// new definition
auto find_unit_name = table.find(unit->get_name());
if (find_unit_name == table.end()) {
table.insert({unit->get_name(), unit});
} else {
table.erase(unit->get_name());
table.insert({unit->get_name(), unit});
}
table.insert({unit->get_name(), unit});
}

void UnitTable::insert_prefix(const std::shared_ptr<Prefix>& prfx) {
Expand Down
21 changes: 1 addition & 20 deletions src/visitors/units_visitor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,23 +81,8 @@ void UnitsVisitor::visit_unit_def(ast::UnitDef& node) {
* care of all the units calculations.
*/
void UnitsVisitor::visit_factor_def(ast::FactorDef& node) {
std::ostringstream ss;
const auto node_has_value_defined_in_modfile = node.get_value() != nullptr;
if (node_has_value_defined_in_modfile) {
/*
* In nrnunits.lib file "1" is defined as "fuzz", so
* there must be a conversion to be able to parse "1" as unit
*/
if (node.get_unit1()->get_node_name() == "1") {
ss << node.get_node_name() << "\t" << node.get_value()->eval() << " ";
ss << UNIT_FUZZ;
} else {
ss << node.get_node_name() << "\t" << node.get_value()->eval() << " ";
ss << node.get_unit1()->get_node_name();
}
// Parse the generated string for the defined unit using the units::UnitParser
units_driver.parse_string(ss.str());
} else {
if (!node_has_value_defined_in_modfile) {
std::ostringstream ss_unit1, ss_unit2;
std::string unit1_name, unit2_name;
/*
Expand Down Expand Up @@ -127,10 +112,6 @@ void UnitsVisitor::visit_factor_def(ast::FactorDef& node) {
ss_unit2 << node.get_node_name() << "_unit2\t" << unit2_name;
units_driver.parse_string(ss_unit2.str());

// Parse the generated string for the defined unit using the units::UnitParser
ss << node.get_node_name() << "\t" << unit1_name;
units_driver.parse_string(ss.str());

/**
* \note If the ast::FactorDef was made by using two units (second case),
* the factors of both of them must be calculated based on the
Expand Down
4 changes: 2 additions & 2 deletions test/unit/units/parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ SCENARIO("Unit parser accepting dependent/nested units definition", "[unit][pars
dummy3 25-3 / m2
dummy4 -0.025 /m2
dummy5 2.5 %
R k-mole
newR k-mole
R1 8.314 volt-coul/degC
R2 8314 mV-coul/degC
)";
Expand All @@ -202,7 +202,7 @@ SCENARIO("Unit parser accepting dependent/nested units definition", "[unit][pars
REQUIRE_THAT(parsed_units, Contains("dummy3 0.025: -2 0 0 0 0 0 0 0 0 0"));
REQUIRE_THAT(parsed_units, Contains("dummy4 -0.025: -2 0 0 0 0 0 0 0 0 0"));
REQUIRE_THAT(parsed_units, Contains("dummy5 0.025: 0 0 0 0 0 0 0 0 0 0"));
REQUIRE_THAT(parsed_units, Contains("R 8.31446: 2 1 -2 0 0 0 0 0 0 -1"));
REQUIRE_THAT(parsed_units, Contains("newR 8.31446: 2 1 -2 0 0 0 0 0 0 -1"));
REQUIRE_THAT(parsed_units, Contains("R1 8.314: 2 1 -2 0 0 0 0 0 0 -1"));
REQUIRE_THAT(parsed_units, Contains("R2 8.314: 2 1 -2 0 0 0 0 0 0 -1"));
REQUIRE_THAT(parsed_units, Contains("m kg sec coul candela dollar bit erlang K"));
Expand Down
Loading

0 comments on commit 5f864dc

Please sign in to comment.