Include median #2049
Include median #2049
Conversation
lrubiorod
force-pushed
the
include_median
branch
from
54e20c3
to
34f7ddf
Compare
| ]); | ||
| // RoundToInteger means that when the average is not an integer, it will be rounded to an | ||
| // integer. For example, the average of 1 and 2, which is 1.5, will be rounded to 1. | ||
| mean(&rl, MeanReturnPolicy::RoundToInteger) |
There was a problem hiding this comment.
According to the documentation of MeanReturnPolicy, RoundToInteger actually means "keep same type as the input":
/// Different available policies regarding what to do with the resulting Float after applying the
/// average mean.
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum MeanReturnPolicy {
/// Pure `Fn(Array<T>) -> Float` behavior.
ReturnFloat,
/// Enforces `Fn(Array<T>) -> T` behavior (if needed).
RoundToInteger,
}
lrubiorod
force-pushed
the
include_median
branch
from
34f7ddf
to
298a5f0
Compare
node/src/actors/chain_manager/mod.rs
Outdated
| && !self | ||
| .chain_state | ||
| .tapi_engine | ||
| .wip_activation | ||
| .contains_key("WIP0017") |
There was a problem hiding this comment.
This condition is new: it means that miners will stop signaling when the WIP is activated. Not sure if this is the desired behavior, because this field is also used as a census to see the percentage of miners that are on the latest version. Also, bit 0 is set to 0 in this PR while currently in mainnet it is set to 1. That doesn't break anything consensus-wise but it may be unexpected.
There was a problem hiding this comment.
In my opinion, this bit only should be used to signaling, and after achieve the necessary votes, it is not longer required. We should remove the activation of this bit, to avoid overlap bit problems in the future
There was a problem hiding this comment.
I have mixed opinions. WIP-0014 does not define what to do because it is consensus neutral. It is true that setting bit position 0 to value 0 at this point may come as a surprise, but also we know for certain that we'll need to do so at some point in the coming years. So maybe it is time to define whether we are OK with setting them to 0 upon next bit position being assigned, or we make a commitment to set all bits to 0 when we assign bit 31 (although that removes the ability of signaling 30 and 31 at the same time).
There was a problem hiding this comment.
I think being able to query which (ARS) nodes have signalled support for a certain WIP after the WIP is passed or defeated is quite a useful feature. Yes, in the (distant) future, bits will be reused and at that point this'll become a problem, but maybe that is not something that should be solved right now.
There was a problem hiding this comment.
Could be an option to leave as before and open an issue with a discussion about that
There was a problem hiding this comment.
Yes, let's just leave it like it was before.
| RadonReducers::AverageMedian => match &context.active_wips { | ||
| Some(active_wips) if active_wips.wip0017() => median::median(input), | ||
| _ => error(), | ||
| }, |
There was a problem hiding this comment.
Here if context.active_wips is None, the WIP is assumed to be not activated. This is the default for external tools that use the radon library, because they may not have access to a witnet node so they don't know the active wips. However we need to ensure that the None case means "enabled" after this WIP is activated.
There was a problem hiding this comment.
What do you think about changing the None case to mean enabled even if the WIP is not activated yet? This way external tools can create and run requests that use the new operators, but if they deploy the request before the WIP is active they will get an UnsupportedReducer error.
There was a problem hiding this comment.
But if they create a request, they try it with this feature and obtain a proper result, but when send to mainnet, it fails... It wasnt a good test to try before...
rad/src/reducers/median.rs
Outdated
| fn test_operate_reduce_median_empty() { | ||
| let input = RadonArray::from(vec![]); | ||
| let output = median(&input).unwrap_err(); | ||
| let expected_error = ModeEmpty; |
There was a problem hiding this comment.
TODO: we use ModeEmpty as the error for when the median reducer is applied over an empty array. Should we create a new error MedianEmpty, or rename ModeEmpty to a more generic EmptyArray error?
There was a problem hiding this comment.
I think EmptyArray sounds better
There was a problem hiding this comment.
Let's take this into account for the eventual WIP, sirs.
There was a problem hiding this comment.
So it looks like this error cannot be easily renamed because it appears in the UnhandledIntercept error message, so changing its name would make some tallies invalid. However we could maybe avoid the UnhandledIntercept by implementing the conversion from RadError to RadonError and assigning an error code to this error. The currently assigned error codes can be found here in the bridge and here in the node.
There was a problem hiding this comment.
I could add a TODO or FIXME and create a new issue, because I think it is out of the scope of this issue. What do you think?
There was a problem hiding this comment.
We can use EmptyArray only in the median by the moment
lrubiorod
force-pushed
the
include_median
branch
from
62e8ce9
to
d8e6969
Compare
lrubiorod
force-pushed
the
include_median
branch
2 times, most recently
from
f620bc2
to
285426c
Compare
| oppose_wip0014, | ||
| oppose_wip0016, | ||
| } = &self.tapi; | ||
| let Tapi { oppose_wip0017 } = &self.tapi; | ||
|
|
||
| let mut v = 0; |
There was a problem hiding this comment.
This should be v = 1, to keep bit 0 enabled?
lrubiorod
force-pushed
the
include_median
branch
from
285426c
to
d248a53
Compare
|
Let's merge this pull request to a new branch, since we don't know the final activation date yet and the WIP is not final yet. |
Close #2047 and #2054