Adjust AI constructions algorithms#2225
Conversation
| namespace sys { | ||
|
|
||
| // US49 StackedCalculationWithExplanations allows uniting number calculations for backend with explanation tooltips for the UI | ||
| class StackedCalculationWithExplanations { |
There was a problem hiding this comment.
Unfortunately, there are some issues with this design. Turning a formula in the code into an instance of this type is a non-trivial performance sacrifice. I do understand the impulse behind it, however, and the idea can be rescued, even if this implementation isn't on the right track. What you want to do is develop a system of expression templates (the technique even has its own wikipedia page: https://en.wikipedia.org/wiki/Expression_templates ) that encodes the formula and any explanatory strings into a type at compile time, which in turn allows the calculation of that expression to be realized such that it can be compiled to the same end result as if it had been written out directly in the source code.
| StackedCalculationWithExplanations(float initialValue = 0.0f) | ||
| : currentValue(initialValue) { | ||
| // Apply a single operation to a value | ||
| static float apply(Operation op, float current, float val) { |
There was a problem hiding this comment.
This is getting closer to having zero overhead, but writing apply as you have means that, when compiled, the code will still be deciding what arithmetical operations to execute at runtime. Instead this logic can me moved into the step_node struct, allowing it to resolve the choice of operation at compile time.
| } else if constexpr(Op == Operation::MULTIPLY) { | ||
| return current * value; | ||
| } else if constexpr(Op == Operation::DIVIDE) { | ||
| if(value == 0.0f) throw std::runtime_error("Division by zero"); |
There was a problem hiding this comment.
exceptions are not enabled in PA. You can call std::abort or std::terminate instead. That said, checking every division instead of just allowing it to produce inf is slower; I would suggest making this just an assert so that it doesn't appear in the release build
|
Note that this PR still doesn't build; it looks like you are missing some necessary includes |
|
It builds now w no merge conflicts |
|
|
||
| // US50AC1 Each factory type is evaluated by its profitability, payback time, and a number of synergies | ||
| // Unprofitable factories have negative scores | ||
| factory_evaluation_stack evaluate_factory_type(sys::state& state, |
There was a problem hiding this comment.
it is not clear to me why this function returns the unevaluated formula rather than the result of calling .getResult() on it, since that is the only thing done with its return value, and it is of course faster just to return the float result.
There was a problem hiding this comment.
The same function is called from the back end to then retrieve the number and from the UI to display full tooltip over the explanations, which is the point of the change in paradigm.
auto factory_type_score = ai::evaluate_factory_type(state, n, mid, p, content, false, 0.3f, 0.5f, 200.f, rich_effect);
for(auto line : factory_type_score.getSteps()) {
if(line.operation == sys::Operation::ADD || line.operation == sys::Operation::SUBTRACT) {
text::add_line(state, contents, line.explanation, text::variable_type::value, text::fp_one_place{ line.value }, 15);
} else if(line.operation == sys::Operation::DIVIDE) {
text::add_line(state, contents, line.explanation, text::variable_type::value, text::fp_one_place{ 1 / line.value }, 15);
} else {
text::add_line(state, contents, line.explanation, text::variable_type::value, text::fp_one_place{ line.value }, 15);
}
}There was a problem hiding this comment.
It may be worthwhile, then, to figure out the result once at construction and store it then, rather than each time it is needed, to avoid the possibility of redundant computations.
| @@ -0,0 +1,150 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
most of this file still doesn't conform to the capitalization and naming conventions of the project
| auto score = evaluate_factory_type(state, nid, mid, pid, type, pop_project, filter_profitability, filter_output_probability_to_buy, filter_payback_time, effective_profit); | ||
|
|
||
| // Build only profitable factories | ||
| if(score.getResult() > 0.f) { |
There was a problem hiding this comment.
referring back to my previous comment; here you can see both that getting the result is the only thing done with it and that because the result itself is not returned it is easy to accidentally fall into the pattern of calculating the result more than once, which does redundant work for no reason
2 participants
No description provided.