From a7f9b7a7f4f2ec084ca2498e833e20ec99ee8237 Mon Sep 17 00:00:00 2001 From: fxpineau Date: Tue, 5 Nov 2024 09:33:38 +0100 Subject: [PATCH] Update the Skymap/MOM code, taking into account Kostya (@hombit) feedback --- Cargo.toml | 3 +- src/lib.rs | 10 +++ src/nested/map/mom/impls/mod.rs | 4 +- src/nested/map/mom/impls/zvec.rs | 124 +++++++++++++------------------ src/nested/map/mom/mod.rs | 69 ++++++++++------- src/nested/map/skymap.rs | 2 + 6 files changed, 113 insertions(+), 99 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 49ee0f6..bd074d0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,8 @@ name = "cdshealpix" version = "0.7.0" authors = ["F.-X. Pineau "] -edition = "2018" +edition = "2021" +rust-version = "1.81" license = "Apache-2.0 OR MIT" readme = "README.md" categories = ["algorithms", "science"] diff --git a/src/lib.rs b/src/lib.rs index 808e081..56df99a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1858,4 +1858,14 @@ mod tests { assert!((d5_mas - 620.3647193082197).abs() < 1e-12); assert!((d6_mas - 591.2475292479544).abs() < 1e-12); } + + #[test] + fn testok_pos_at_depth29() { + let ra_deg = 45.00432028915398_f64; + let de_deg = 0.021047763781174733_f64; + assert_eq!( + 29_640_498_453, + nested::hash(29, ra_deg.to_radians(), de_deg.to_radians()) + ); + } } diff --git a/src/nested/map/mom/impls/mod.rs b/src/nested/map/mom/impls/mod.rs index 8d07ea2..bed54f9 100644 --- a/src/nested/map/mom/impls/mod.rs +++ b/src/nested/map/mom/impls/mod.rs @@ -1,4 +1,4 @@ //! Module containing MOM implementations in sub-modules -pub mod zvec; -pub mod ported_from_mom_builder; \ No newline at end of file +pub mod ported_from_mom_builder; +pub mod zvec; diff --git a/src/nested/map/mom/impls/zvec.rs b/src/nested/map/mom/impls/zvec.rs index ab14372..8d511ef 100644 --- a/src/nested/map/mom/impls/zvec.rs +++ b/src/nested/map/mom/impls/zvec.rs @@ -1,9 +1,8 @@ -use std::cmp::Ordering; -use std::{iter::Map, slice::Iter, vec::IntoIter}; +use std::{cmp::Ordering, iter::Map, slice::Iter, vec::IntoIter}; use super::super::{ super::skymap::{SkyMap, SkyMapValue}, - Mom, ZUniqHashT, + LhsRhsBoth, Mom, ZUniqHashT, }; /// Implementation of a MOM in an ordered vector of `(zuniq, values)` tuples. @@ -113,7 +112,7 @@ where fn from_skymap_ref<'s, S, M>(skymap: &'s S, merger: M) -> Self where S: SkyMap<'s, HashType = Z, ValueType = V>, - M: Fn(&V, &V, &V, &V) -> Option, + M: Fn(u8, Z, [&V; 4]) -> Option, V: 's, { let depth = skymap.depth(); @@ -128,11 +127,17 @@ where // (among the 4 possible values 0, 1, 2 and 3). if h == expected_next_hash && h & Z::LAST_LAYER_MASK == Z::LAST_LAYER_MASK { let n = entries.len(); + let parent_depth = depth - 1; + let parent_h = h >> 2; if let Some(combined_value) = merger( - &entries[n - 4].1, // sibling 0 - &entries[n - 3].1, // sibling 1 - &entries[n - 2].1, // sibling 2 - &entries[n - 1].1, // sibling 3 + parent_depth, + parent_h, + [ + &entries[n - 4].1, // sibling 0 + &entries[n - 3].1, // sibling 1 + &entries[n - 2].1, // sibling 2 + &entries[n - 1].1, // sibling 3 + ], ) { let _ = entries.pop(); let _ = entries.pop(); @@ -140,7 +145,7 @@ where // Unwrap ok here since we are sure that the array contained at least 4 entries // (we access them just above). let _ = entries.pop().unwrap(); - let new_zuniq = Z::to_zuniq(depth - 1, h >> 2); + let new_zuniq = Z::to_zuniq(parent_depth, parent_h); entries.push((new_zuniq, combined_value)); Self::from_skymap_ref_recursive(&mut entries, &merger); } @@ -155,20 +160,7 @@ where fn from_skymap<'s, S, M>(skymap: S, merger: M) -> Self where S: SkyMap<'s, HashType = Self::ZUniqHType, ValueType = Self::ValueType>, - M: Fn( - Self::ValueType, - Self::ValueType, - Self::ValueType, - Self::ValueType, - ) -> Result< - Self::ValueType, - ( - Self::ValueType, - Self::ValueType, - Self::ValueType, - Self::ValueType, - ), - >, + M: Fn(u8, Z, [V; 4]) -> Result, Self::ValueType: 's, { let depth = skymap.depth(); @@ -184,13 +176,15 @@ where let (z2, v2) = entries.pop().unwrap(); let (z1, v1) = entries.pop().unwrap(); let (z0, v0) = entries.pop().unwrap(); - match merger(v0, v1, v2, v3) { + let parent_depth = depth - 1; + let parent_h = h >> 2; + match merger(parent_depth, parent_h, [v0, v1, v2, v3]) { Ok(v_merged) => { - let z_merged = Z::to_zuniq(depth - 1, h >> 2); + let z_merged = Z::to_zuniq(parent_depth, parent_h); entries.push((z_merged, v_merged)); Self::from_skymap_recursive(&mut entries, &merger); } - Err((v0, v1, v2, v3)) => { + Err([v0, v1, v2, v3]) => { entries.push((z0, v0)); entries.push((z1, v1)); entries.push((z2, v2)); @@ -210,29 +204,9 @@ where where L: Mom<'s, ZUniqHType = Z, ValueType = V>, R: Mom<'s, ZUniqHType = Z, ValueType = V>, - S: Fn( - Self::ValueType, - ) -> ( - Self::ValueType, - Self::ValueType, - Self::ValueType, - Self::ValueType, - ), - O: Fn(Option, Option) -> Option, - M: Fn( - Self::ValueType, - Self::ValueType, - Self::ValueType, - Self::ValueType, - ) -> Result< - Self::ValueType, - ( - Self::ValueType, - Self::ValueType, - Self::ValueType, - Self::ValueType, - ), - >, + S: Fn(u8, Z, V) -> [V; 4], + O: Fn(LhsRhsBoth) -> Option, + M: Fn(u8, Z, [V; 4]) -> Result, { #[derive(Clone)] struct DHZ { @@ -282,7 +256,7 @@ where (Some(l), Some(r)) => match l.d.cmp(&r.d) { Ordering::Equal => match l.h.cmp(&r.h) { Ordering::Equal => { - if let Some(v) = op(Some(l.v), Some(r.v)) { + if let Some(v) = op(LhsRhsBoth::Both(l.v, r.v)) { last_dhz_added = DHZ::new(l.d, l.h, l.z); stack.push((l.z, v)); } @@ -298,7 +272,7 @@ where }; } Ordering::Less => { - if let Some(v) = op(Some(l.v), None) { + if let Some(v) = op(LhsRhsBoth::Left(l.v)) { last_dhz_added = DHZ::new(l.d, l.h, l.z); stack.push((l.z, v)); } @@ -310,7 +284,7 @@ where right = Some(r); } Ordering::Greater => { - if let Some(v) = op(None, Some(r.v)) { + if let Some(v) = op(LhsRhsBoth::Right(r.v)) { last_dhz_added = DHZ::new(r.d, r.h, r.z); stack.push((r.z, v)); } @@ -328,7 +302,7 @@ where let r_hash_at_l_depth = r.h >> (((r.d - l.d) << 1) as usize); match l.h.cmp(&r_hash_at_l_depth) { Ordering::Equal => { - let (v0, v1, v2, v3) = split(l.v); + let [v0, v1, v2, v3] = split(l.d, l.h, l.v); let new_d = l.d + 1; let new_h = l.h << 2; lstack.push(DHZV::new( @@ -353,7 +327,7 @@ where right = Some(r); } Ordering::Less => { - if let Some(v) = op(Some(l.v), None) { + if let Some(v) = op(LhsRhsBoth::Left(l.v)) { last_dhz_added = DHZ::new(l.d, l.h, l.z); stack.push((l.z, v)); } @@ -365,7 +339,7 @@ where right = Some(r); } Ordering::Greater => { - if let Some(v) = op(None, Some(r.v)) { + if let Some(v) = op(LhsRhsBoth::Right(r.v)) { last_dhz_added = DHZ::new(r.d, r.h, r.z); stack.push((r.z, v)); } @@ -382,7 +356,7 @@ where let l_has_at_r_depth = l.h >> (((l.d - r.d) << 1) as usize); match l_has_at_r_depth.cmp(&r.h) { Ordering::Equal => { - let (v0, v1, v2, v3) = split(r.v); + let [v0, v1, v2, v3] = split(r.d, r.h, r.v); let new_d = r.d + 1; let new_h = r.h << 2; rstack.push(DHZV::new( @@ -407,7 +381,7 @@ where right = Some(DHZV::new(new_d, new_h, Z::to_zuniq(new_d, new_h), v0)); } Ordering::Less => { - if let Some(v) = op(Some(l.v), None) { + if let Some(v) = op(LhsRhsBoth::Left(l.v)) { last_dhz_added = DHZ::new(l.d, l.h, l.z); stack.push((l.z, v)); } @@ -419,7 +393,7 @@ where right = Some(r); } Ordering::Greater => { - if let Some(v) = op(None, Some(r.v)) { + if let Some(v) = op(LhsRhsBoth::Right(r.v)) { last_dhz_added = DHZ::new(r.d, r.h, r.z); stack.push((r.z, v)); } @@ -436,7 +410,7 @@ where (None, None) => break, // Important to let this test here!!! (mut left, None) => { while let Some(l) = left { - if let Some(v) = op(Some(l.v), None) { + if let Some(v) = op(LhsRhsBoth::Left(l.v)) { last_dhz_added = DHZ::new(l.d, l.h, l.z); stack.push((l.z, v)); } @@ -459,7 +433,7 @@ where } (None, mut right) => { while let Some(r) = right { - if let Some(v) = op(None, Some(r.v)) { + if let Some(v) = op(LhsRhsBoth::Right(r.v)) { last_dhz_added = DHZ::new(r.d, r.h, r.z); stack.push((r.z, v)); } @@ -515,7 +489,7 @@ where fn from_skymap_ref_recursive<'s, M>(stack: &mut Vec<(Z, V)>, merger: &M) where - M: Fn(&V, &V, &V, &V) -> Option, + M: Fn(u8, Z, [&V; 4]) -> Option, V: 's, { let n = stack.len(); @@ -530,17 +504,23 @@ where && e2.0 == Z::to_zuniq(d0, h0 + Z::two()) && e3.0 == Z::to_zuniq(d0, h0 + Z::three()) { + let parent_depth = d0 - 1; + let parent_h = h0 >> 2; if let Some(combined_value) = merger( - &e0.1, // sibling 0 - &e1.1, // sibling 1 - &e2.1, // sibling 2 - &e3.1, // sibling 3 + parent_depth, + parent_h, + [ + &e0.1, // sibling 0 + &e1.1, // sibling 1 + &e2.1, // sibling 2 + &e3.1, // sibling 3 + ], ) { let _ = stack.pop().unwrap(); let _ = stack.pop().unwrap(); let _ = stack.pop().unwrap(); let _ = stack.pop().unwrap(); - let new_zuniq = Z::to_zuniq(d0 - 1, h0 >> 2); + let new_zuniq = Z::to_zuniq(parent_depth, parent_h); stack.push((new_zuniq, combined_value)); Self::from_skymap_ref_recursive(stack, merger); } @@ -551,7 +531,7 @@ where fn from_skymap_recursive<'s, M>(stack: &mut Vec<(Z, V)>, merger: &M) where - M: Fn(V, V, V, V) -> Result, + M: Fn(u8, Z, [V; 4]) -> Result, V: 's, { let n = stack.len(); @@ -570,13 +550,15 @@ where let (_, v2) = stack.pop().unwrap(); let (_, v1) = stack.pop().unwrap(); let (_, v0) = stack.pop().unwrap(); - match merger(v0, v1, v2, v3) { + let parent_depth = d0 - 1; + let parent_h = h0 >> 2; + match merger(parent_depth, parent_h, [v0, v1, v2, v3]) { Ok(v_merged) => { let z_merged = Z::to_zuniq(d0 - 1, h0 >> 2); stack.push((z_merged, v_merged)); Self::from_skymap_recursive(stack, merger); } - Err((v0, v1, v2, v3)) => { + Err([v0, v1, v2, v3]) => { stack.push((z0, v0)); stack.push((z1, v1)); stack.push((z2, v2)); @@ -612,7 +594,7 @@ mod tests { let skymap = SkyMapEnum::from_fits_file(path).unwrap(); match skymap { SkyMapEnum::ImplicitU64I32(skymap) => { - let merger = |n0: &i32, n1: &i32, n2: &i32, n3: &i32| -> Option { + let merger = |_depth: u8, _hash: u64, [n0, n1, n2, n3]: [&i32; 4]| -> Option { let sum = *n0 + *n1 + *n2 + *n3; if sum < 1_000_000 { Some(sum) @@ -670,7 +652,7 @@ mod tests { SkyMapEnum::ImplicitU64I32(skymap) => { // println!("Skymap size: {}", skymap.len()); - let merger = |n0: &i32, n1: &i32, n2: &i32, n3: &i32| -> Option { + let merger = |_depth: u8, _hash: u64, [n0, n1, n2, n3]: [&i32; 4]| -> Option { let mu0 = *n0 as f64; // let sig0 = mu0.sqrt(); let mu1 = *n1 as f64; diff --git a/src/nested/map/mom/mod.rs b/src/nested/map/mom/mod.rs index 4080bb6..a5693d1 100644 --- a/src/nested/map/mom/mod.rs +++ b/src/nested/map/mom/mod.rs @@ -119,6 +119,16 @@ impl ZUniqHashT for u64 { } } +/// Enum defining the 3 possibilities whe merging two nullable inputs: +/// * Left: non-null LHS, null RHS +/// * Right: null LHS, non-null RHS +/// * Both: non-null LHS, non-null RHS +pub enum LhsRhsBoth { + Left(V), + Right(V), + Both(V, V), +} + /// `MOM` stands for **M**ulti **O**rder healpix **M**aps. /// Here, it consists in a list of HEALPix cells (hash) at various depth (HEALPixordes) /// with a value attached to each cell. @@ -220,7 +230,7 @@ pub trait Mom<'a>: Sized { fn from_skymap_ref<'s, S, M>(skymap: &'s S, merger: M) -> Self where S: SkyMap<'s, HashType = Self::ZUniqHType, ValueType = Self::ValueType>, - M: Fn(&Self::ValueType, &Self::ValueType, &Self::ValueType, &Self::ValueType) -> Option, + M: Fn(u8, Self::ZUniqHType, [&Self::ValueType; 4]) -> Option, Self::ValueType: 's; /// # Params @@ -228,25 +238,33 @@ pub trait Mom<'a>: Sized { fn from_skymap<'s, S, M>(skymap: S, merger: M) -> Self where S: SkyMap<'s, HashType = Self::ZUniqHType, ValueType = Self::ValueType>, - M: Fn(Self::ValueType, Self::ValueType, Self::ValueType, Self::ValueType) -> Result, + M: Fn(u8, Self::ZUniqHType, [Self::ValueType; 4]) -> Result, Self::ValueType: 's; - // This is a first version, but: - // * the pixel depth and hash value should probably be passed to the 'split' method - // (in case the splitting strategy depends on the sub-cell, i.e. splitting a list of positions + /// Merge two input MOMs to produce an output MOM. + /// # Params + /// * `lhs`: the Left Hand Side MOM + /// * `rhs`: the right Hand side MOM + /// * `split`: the operator splitting a lhs or rhs cell into its four direct siblings + /// + the depth and hash value of the parent cell are provided in input. + /// + together with the value of the cell to be split + /// * `op`: merge/mixe/join the values of a same cell from both input MOMs (or decide what to do + /// when one of the MOM cell has a value and the other does not). + /// + it allows for various types of JOIN (inner, left, right, full). + /// * `merge`: possibly performs a post-merge operation, considering the (four) siblings of the join operation. + /// + the depth and hash value of the parent cell (if cells are merged) are provided in input, + /// + together with the 4 values to be possibly merged in the parent cell. + /// + result if either `Ok` with the merged value if a merge occurs, or `Err` with the input values if + /// the merge does not occurs. + /// * `L`: type of the left MOM + /// * `R`: type of the right MOM fn merge<'s, L, R, S, O, M>(lhs: L, rhs: R, split: S, op: O, merge: M) -> Self where L: Mom<'s, ZUniqHType = Self::ZUniqHType, ValueType = Self::ValueType>, R: Mom<'s, ZUniqHType = Self::ZUniqHType, ValueType = Self::ValueType>, - // Split a parent cell into four siblings - S: Fn(Self::ValueType) -> (Self::ValueType, Self::ValueType, Self::ValueType, Self::ValueType), - // Merge the values of a same cell from both MOMs - // or decide what to do is one of the MOM has a value and the other does not - // (the (None, None) must never be called). - // This allow for various types of JOIN (inner, left, right, full). - O: Fn(Option, Option) -> Option, - // Possibly performs a post-merge operation. - M: Fn(Self::ValueType, Self::ValueType, Self::ValueType, Self::ValueType) -> Result; + S: Fn(u8, Self::ZUniqHType, Self::ValueType) -> [Self::ValueType; 4], + O: Fn(LhsRhsBoth) -> Option, + M: Fn(u8, Self::ZUniqHType, [Self::ValueType; 4]) -> Result; } @@ -256,6 +274,7 @@ pub trait Mom<'a>: Sized { mod tests { use mapproj::pseudocyl::mol::Mol; use crate::nested::map::img::to_mom_png_file; + use crate::nested::map::mom::LhsRhsBoth; use super::{ Mom, @@ -274,7 +293,7 @@ mod tests { let path = "test/resources/skymap/skymap.gaiadr3.depth6.fits"; let skymap_gaia = SkyMapEnum::from_fits_file(path).unwrap(); - let chi2_merger = |n0: &i32, n1: &i32, n2: &i32, n3: &i32| -> Option { + let chi2_merger = |_depth: u8, _hash: u64, [n0, n1, n2, n3]: [&i32; 4]| -> Option { let mu0 = *n0 as f64; // let sig0 = mu0.sqrt(); let mu1 = *n1 as f64; @@ -355,24 +374,24 @@ mod tests { (n_div_4, n_div_4 + a, n_div_4 + b, n_div_4 + c) }; */ - let split = |pdf: f64| { + let split = |_depth: u8, _hash: u64, pdf: f64| { let pdf_div_4 = pdf / 4.0; - (pdf_div_4, pdf_div_4, pdf_div_4, pdf_div_4) + [pdf_div_4, pdf_div_4, pdf_div_4, pdf_div_4] }; - let op = |l: Option, r: Option| { - let l = l.unwrap_or(0.0); - let r = r.unwrap_or(0.0); - Some(l - r) - // Some(l) + let op = |lrb: LhsRhsBoth| { + match lrb { + LhsRhsBoth::Left(l) => Some(l), + LhsRhsBoth::Right(r) => Some(r), + LhsRhsBoth::Both(l, r) => Some(l - r), + } }; - let merge = |v1: f64, v2: f64, v3: f64, v4: f64| { + let merge = |_depth: u8, _hash: u64, [v1, v2, v3, v4]: [f64; 4]| { let sum = v1 + v2 + v3 + v4; if sum.abs() < 1e-4 { Ok(sum) } else { - Err((v1, v2, v3, v4)) + Err([v1, v2, v3, v4]) } - // Err((v1, v2, v3, v4)) }; let mom = MomVecImpl::merge(mom_2mass, mom_gaia, split, op, merge); to_mom_png_file::<'_, _, Mol, _>( diff --git a/src/nested/map/skymap.rs b/src/nested/map/skymap.rs index b47a130..9770cce 100644 --- a/src/nested/map/skymap.rs +++ b/src/nested/map/skymap.rs @@ -265,6 +265,8 @@ impl<'a, H: HHash, V: SkyMapValue + Clone + 'a> SkyMap<'a> for ImplicitSkyMapArr .map(move |(h, v)| (H::from_usize(h), v)) } + // Make a owned_values method!! + fn owned_entries(self) -> Self::OwnedEntriesIt { self .values