[ML] By-fields should respect model_plot_config.terms #86
Conversation
This commit restores respecting the model_plot_config.terms for by-fields. The bug seems to have been introduced in 6.2.0. Note that I am also adding an integration test in the java side to ensure such regressions are caught in the future. Closes elastic/elasticsearch#30004
dimitris-athanasiou
force-pushed
the
respect-model-plot-terms-for-by-field
branch
from
1b3edb0 to
3bd9439
Compare
lib/model/CModelDetailsView.cc
Outdated
| @@ -159,7 +159,7 @@ bool CModelDetailsView::contains(const TStrSet& terms, const std::string& key) { | |||
| } | |||
|
|
|||
| bool CModelDetailsView::hasByField() const { | |||
| return (this->base().isPopulation() | |||
| return !(this->base().isPopulation() | |||
There was a problem hiding this comment.
wouldn't it be easier to switch the conditional expressions?
! is always hard to parse and probably the root of this regression?
There was a problem hiding this comment.
obviously hard, I overlooked the brackets. Maybe == false - I think we recently discussed to use this in favor of !
There was a problem hiding this comment.
Do you mean changing this method into hasNoByField and then inverting the conditionals where it's called from? I'd rather not do that. But we could certainly change the function to the format (...).empty() == false. Do you prefer this?
|
LGTM Any chance to create a testcase for this? If I got it right, this is an uncovered case. |
lib/model/CModelDetailsView.cc
Outdated
| @@ -162,7 +162,7 @@ bool CModelDetailsView::hasByField() const { | |||
| return (this->base().isPopulation() | |||
| ? this->base().dataGatherer().attributeFieldName() | |||
| : this->base().dataGatherer().personFieldName()) | |||
| .empty(); | |||
| .empty() == false; | |||
There was a problem hiding this comment.
The other thing that might possibly improve readability is to split this into two statements. Storing the result of the ternary operator in a const std::string & local variable and then returning a condition based on this in a second statement won't make the code less efficient once the optimiser has run on it, as the optimiser can easily see that the const std::string & can live in a register and never written back to memory.
And regardless of whether you decide to make that change, have you run clang-format on the file?
There was a problem hiding this comment.
Happy to split the statement.
I have run it. It's clang-format that indented the line I changed. Not sure why. I imagine it will change again once I split the statement.
|
@hendrikmuhs I looked into creating a unit test but it wasn't straightforward. Instead, I am writing an integration test in the java side. I've been thinking to create such a test for a long time for model plots, so it's a good opportunity. |
|
The only other thing to watch out for with this is that 6.3.0 BC3 has started. I fully expect there to be a BC4 but if there isn't then this will end up in 6.3.1. I will keep an eye on 6.3.0 progress. |
This commit restores respecting the model_plot_config.terms for by-fields. The bug seems to have been introduced in 6.2.0. Note that I am also adding an integration test in the java side to ensure such regressions are caught in the future. Closes elastic/elasticsearch#30004
This commit restores respecting the model_plot_config.terms for by-fields. The bug seems to have been introduced in 6.2.0. Note that I am also adding an integration test in the java side to ensure such regressions are caught in the future. Closes elastic/elasticsearch#30004
This commit restores respecting the model_plot_config.terms
for by-fields. The bug seems to have been introduced in 6.2.0.
Note that I am also adding an integration test in the java
side to ensure such regressions are caught in the future.
Closes elastic/elasticsearch#30004