Feature/scaling action #38
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #38 +/- ##
===========================================
- Coverage 23.18% 23.10% -0.08%
===========================================
Files 284 290 +6
Lines 30216 30312 +96
Branches 1104 1118 +14
===========================================
Hits 7005 7005
- Misses 23211 23307 +96 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
dsarmany
force-pushed
the
feature/scaling-action
branch
from
adbfc0f
to
03cda38
Compare
src/multio/action/scale/Scale.cc
Outdated
| } | ||
|
|
||
| std::string cparam{"xxx"}; | ||
| if (auto param = msg.metadata().getOpt<std::string>(glossary().param); param) { |
There was a problem hiding this comment.
I don't think we need the ; param bit. If we use assignment in the if, the result is whether param has a valid value or not. But this practice is, anyway, questionable. But I'm personally fine with it.
There was a problem hiding this comment.
This I got from Mirco as he was working on similar logic for his statistic class. I did some reading on this. It's true since I only use param and paramID within the if clause it should work but I think with cpp 17 the recommentation is to use as this?
src/multio/action/scale/Scale.h
Outdated
| std::set<std::string> paramsToScale_; | ||
|
|
||
| template <typename Precision> | ||
| message::Message ScaleMessage(message::Message&& msg) const; |
There was a problem hiding this comment.
Why not just pass by lvalue reference and return void. I think it is simpler and safer in this case.
There was a problem hiding this comment.
sorry overlooked the first time I checked the comments. I agree can be reference. I'll try and test that
| return; | ||
| } | ||
| // Apply the scaling factor using std::transform | ||
| std::transform(data, data + size, data, |
There was a problem hiding this comment.
In this case, std::for_each is sufficient. It is simpler and thus preferable, I'd say.
There was a problem hiding this comment.
I think I disagree in this case, I'm sorry. I think transform makes it clearer as to the context of the underlying action (which in fact is a transformation of data in place). I agree it could be done with for_each but I don't think it would be simpler. Nevertheless I'm also not very partial on this line of code. So if you really prefer for_each I can change the line to
std::for_each(data, data + size, [scaleFactor](Precision& value) { value = static_cast<Precision>(value * scaleFactor); });
|
|
||
| ScaleScaling::ScaleScaling(const config::ComponentConfiguration& compConf) : hasScaling_(false), scaleFactor_{} { | ||
|
|
||
| const auto mappings = compConf.parsedConfig().has("mapping-definition") |
There was a problem hiding this comment.
There seems to be some duplication of logic here as well. Can we remove the one in Scale.cc?
There was a problem hiding this comment.
I was weighing that option: I can either create individual plans for fields where scaling is needed or as I currently have add it to plans where only a subset of fields actually need scaling specified by case in the plan. BUT then I wanted to have a check to leave this action as early as possible. I can move the logic in a util function and call it, but this would still be in all three cases. Or I can remove the whole logic of checking as soon as possible. Or I can retrieve the param only in scale.cc and then pass it to the apply function (But this I didn't prefer since I wanted to have the Mapping and scaling as enclosed as possbile sind I pass the metadata anyway and hence have the information anyway)
src/multio/action/scale/Mapping.cc
Outdated
|
|
||
| void ScaleMapping::applyMapping(message::Metadata& md) const { | ||
| if (hasMapping_) { | ||
| std::string cparam{"xxx"}; |
There was a problem hiding this comment.
Same comments here about the logic and duplication (triplication). I'm a bit confused. I think we need this in two places but not in three?
There was a problem hiding this comment.
see explanation above. Happy to change but would like to discuss what makes the most sense
Adding scaling action for Climate DT Phase 2 portfolio.