refac(pallets/subspace): making code more robust #38
Conversation
|
I would rename the title so the name of the squash commit would be more explicit |
pallets/subspace/src/math.rs
Outdated
| for i in x.iter_mut() { | ||
| *i /= x_sum; | ||
| if x_sum != I32F32::from_num(0) { | ||
| x.iter_mut().for_each(|i| *i /= x_sum); |
There was a problem hiding this comment.
I would still use for instead of for_each
pallets/subspace/src/step.rs
Outdated
|
|
||
| if is_founder_registered { | ||
| let founder_share: u16 = Self::get_founder_share(netuid); | ||
| if founder_share > 0 { |
There was a problem hiding this comment.
The same thing here with nested ifs
There was a problem hiding this comment.
I think we should replace this prints with asserts instead of just replacing with logs
There was a problem hiding this comment.
But this can be left for further and another PRs
|
d4b3f23 |
| @@ -31,7 +31,6 @@ impl<T: Config> Pallet<T> { | |||
| } | |||
| } | |||
|
|
|||
| // TODO: make sure there are checks for all values | |||
pallets/subspace/src/global.rs
Outdated
| @@ -99,6 +98,9 @@ impl<T: Config> Pallet<T> { | |||
| } | |||
|
|
|||
| pub fn set_global_params(params: GlobalParams) { | |||
| // Check if the params are valid | |||
| Self::check_global_params(params.clone()).unwrap(); | |||
There was a problem hiding this comment.
I'm pretty sure check_global_params could receive &GlobalParams instead and avoid cloning it here.
Change it to an expect statement. Unwraps are usually unhelpful.
| Self::check_global_params(params.clone()).unwrap(); | |
| Self::check_global_params(params.clone()).expect("global params are invalid"); |
pallets/subspace/src/lib.rs
Outdated
There was a problem hiding this comment.
Were the storage items removed or relocated? I can't find Key2Controller. This will most probably mess with the genesis building stage, was this tested?
pallets/subspace/src/math.rs
Outdated
| @@ -43,12 +43,12 @@ pub fn mask_diag_sparse(sparse_matrix: &[Vec<(u16, I32F32)>]) -> Vec<Vec<(u16, I | |||
|
|
|||
| /// Normalizes (sum to 1 except 0) each row (dim=0) of a sparse matrix in-place. | |||
| pub fn inplace_row_normalize_sparse(sparse_matrix: &mut [Vec<(u16, I32F32)>]) { | |||
| for sparse_row in sparse_matrix.iter_mut() { | |||
| sparse_matrix.iter_mut().for_each(|sparse_row| { | |||
There was a problem hiding this comment.
A for statement is the idiomatic way to do this here, we can leave it the way it was
pallets/subspace/src/profit_share.rs
Outdated
| let normalized_share = | ||
| (share as u64 * u16::MAX as u64 / total_shares as u64) as u16; | ||
| normalized_share |
There was a problem hiding this comment.
| let normalized_share = | |
| (share as u64 * u16::MAX as u64 / total_shares as u64) as u16; | |
| normalized_share | |
| (share as u64 * u16::MAX as u64 / total_shares as u64) as u16 |
|
@saiintbrisson |
Supremesource
changed the title
| if sum == 0 { | ||
| return weights; | ||
| } | ||
| weights.iter_mut().for_each(|x| { |
There was a problem hiding this comment.
use for instead of for_each
|
@aripiprazole |
Supremesource
changed the title
Simplifying the code, fixing clones, making more readable