Add data preparation functions for the Kr map computation by bpalmeiro · Pull Request #865 · next-exp/IC

bpalmeiro · 2024-03-07T12:04:36Z

This PR links to Issue #862 and presents a proposal for the set of functions to perform the data selection and checking for the preparation for the Kr map creation. However, there are a couple of things pending up for discussion:

The band selection relies on the lt functions that we considered adding in the map_creation PR (since we don't think they belong in this PR workflow), but this is convoluted. It may be worth adding a previous PR with those functions.
The previous point makes the band selection function only a scheme of the final implementation, being the only function not tested for the present PR.
The workflow related to implementing this function in the city was left for the general PR —or even a posterior PR—related to the city itself. We considered that this makes this one much clearer since it relates only to adding the functions. Thoughts?

jahernando · 2024-04-04T15:18:45Z

Some comments:

check_if_values_in_interval seems a decorator of in_range, can be done in a more elegant way?
apply_geo_correction is not a good name: something more as get_xy_geo_correction_function
apply_geo_correction is a useful wrapper, but what is a ASectorMap object? Do we need it? Do you keep it for backward compatibility?
selection_nS_mask_and_checking is not a good name, something as check_nS_efficiency_is_in_range, add comment that what are the values, I guess the range interval of the allowed efficiencies on S1 and S2.
band_selector_and_check, replace name for select_and_check_S2signal_vs_dtime_band
selection_in_band this is a generic function, maybe it is better in a more general scope, it is in fact an operation over a 2d profile.

Tests:

ok check_if_values_in_interval
ok apply_geo_correction
ok selection_nS_mask_and_checking

gonzaponte

First round. This is being done with peras, more or less.

gonzaponte

A more in-depth review now. Mostly cosmetics though.

gonzaponte · 2024-12-30T16:44:51Z

+        True if values are in the interval. False if not and strictness
+        is set to 'warning'. Otherwise, it raises an exception.
+    """
+


Remove blank line.

Still there

And alignment accordingly

gonzaponte

Mostly cosmetics

gonzaponte · 2025-01-23T10:16:13Z

+        True if values are in the interval. False if not and strictness
+        is set to 'warning'. Otherwise, it raises an exception.
+    """
+


Still there

gonzaponte · 2025-01-23T10:16:28Z

+
+    **kwargs:
+        Optional arguments being passed to `in_range`.
+    Returns


Suggested change

Returns

Returns

add blank line before Returns

gonzaponte · 2025-01-23T10:19:48Z

+    norm_value : Float(optional)
+        If norm_strat is selected to be custom, user must provide the
+        desired scale.
+    Returns


Add blank line before Returns

gonzaponte · 2025-01-23T10:22:32Z

+    norm_strat : NormStrategy
+        Provides the desired normalization to be used.
+    norm_value : Float(optional)
+        If norm_strat is selected to be custom, user must provide the
+        desired scale.


Suggested change

norm_strat : NormStrategy

Provides the desired normalization to be used.

norm_value : Float(optional)

If norm_strat is selected to be custom, user must provide the

desired scale.

norm_strat : NormStrategy, optional

Provides the desired normalization to be used. Default: NormStrategy.max.

norm_value : float, optional

If norm_strat is selected to be custom, user must provide the

desired scale.

gonzaponte · 2025-01-23T10:23:59Z

+@given(float_arrays(size      = 1, min_value = -198, max_value =      198),
+       float_arrays(size      = 1, min_value = -198, max_value =      198),
+       float_arrays(size      = 1, min_value =    0, max_value =      500),
+       float_arrays(size      = 1, min_value =    0, max_value = 100*1000))


Remove unnecessary spaces after "size"

gonzaponte · 2025-01-23T10:27:40Z

+                             input_mask   : Optional[np.ndarray]  = None                 ,
+                             eff_interval : Optional[Tuple[float, float]] = [0,1]        ,


I would remove the default value for these two arguments (and thus their Optional type hint)

gonzaponte · 2025-01-23T10:30:30Z

+                          norm_strat  : Optional[NormStrategy]                  = NormStrategy.max,
+                          input_mask  : Optional[np.ndarray]                    = None            ,
+                          range_DT    : Optional[Tuple[np.ndarray, np.ndarray]] = (10, 1300)      ,
+                          range_E     : Optional[Tuple[np.ndarray, np.ndarray]] = (10.0e+3,14e+3) ,
+                          nsigma_sel  : Optional[float]                         = 3.5             ,
+                          eff_interval: Optional[Tuple[float, float]]           = [0,1]           ,
+                          strictness  : Optional[Strictness]                    = Strictness.raise_error


Of these, I think nsigma_sel and strictness could/should be optional, the rest I prefer that they are not. I'm not sure about norm_strat, because there might be some assumptions in the code that are optimized for that choice. I think you will have a better feeling for that one.

gonzaponte · 2025-01-23T10:32:13Z

+    nsigma_sel: float (Optional)
+        Number of sigmas to set the band width.
+        Defaults to 3.5.
+    eff_interval (Optional)


Suggested change

eff_interval (Optional)

eff_interval: (float, float), optional

Suggested change

eff_interval (Optional)

eff_interval: Tuple[float, float], optional

Please keep on of these formats everywhere to make the style of the documentation consistent. We are heavily inspired by numpy's documentation, so in case of doubt, that is the reference.

gonzaponte · 2025-01-23T10:34:01Z

+def select_band(dt        : np.ndarray         ,
+                e         : np.ndarray         ,
+                range_dt  : Tuple[float, float],
+                range_e   : Tuple[float, float],
+                nsigma    : Optional[float] = 3.5) ->np.ndarray:


Suggested change

def select_band(dt : np.ndarray ,

e : np.ndarray ,

range_dt : Tuple[float, float],

range_e : Tuple[float, float],

nsigma : Optional[float] = 3.5) ->np.ndarray:

def select_band(dt : np.ndarray ,

e : np.ndarray ,

range_dt : Tuple[float, float],

range_e : Tuple[float, float],

nsigma : Optional[float] = 3.5) ->np.ndarray:

gonzaponte · 2025-01-23T10:37:48Z

+    nsigma: float (Optional)
+        Number of sigmas to set the band width
+        Defaults to 3.5
+    Returns


Add blank line before Returns

bpalmeiro requested review from gonzaponte and jahernando March 7, 2024 12:04

bpalmeiro linked an issue Mar 7, 2024 that may be closed by this pull request

ICAROS: data preparation #862

Open

3 tasks

bpalmeiro force-pushed the data_preparation branch from 16d65f1 to 0741041 Compare March 8, 2024 16:07

bpalmeiro changed the title ~~Data preparation~~ Add data preparation functions for the Kr map computation Mar 8, 2024

bpalmeiro assigned bpalmeiro and carhc Mar 8, 2024

bpalmeiro added the enhancement label Mar 8, 2024

carhc mentioned this pull request Mar 8, 2024

Add main functions for the computation of Kr maps #866

Draft

bpalmeiro force-pushed the data_preparation branch from 0741041 to b0f7f55 Compare December 26, 2024 12:40

bpalmeiro added 10 commits December 26, 2024 13:47

Add Strickness symbol

655182a

Add ValueOutOfRange exception

10affe1

Add check_if_values_in_interval function

528ad25

Add tests for check_if_values_in_interval function

b64f9e5

Add type_of_signal enumerate

3075a1a

Add apply_geo_correction function

cd737bc

Add test for apply_geo_correction function

e1538aa

Add seleccion_nS_mask_and_checking function

00a3b14

Add tests for selection_nS_mask_and_checking

e646b80

Add band_selector_and_check function

73157bd

bpalmeiro force-pushed the data_preparation branch 4 times, most recently from d287075 to 4578a4b Compare December 27, 2024 11:30

gonzaponte reviewed Dec 27, 2024

View reviewed changes

bpalmeiro force-pushed the data_preparation branch 3 times, most recently from 9520347 to a898d85 Compare December 30, 2024 11:28

Add selection_in_band function

378dcb9

Cosmetics and types

9871e66

gonzaponte reviewed Jan 2, 2025

View reviewed changes

bpalmeiro added 4 commits January 14, 2025 11:25

Update typehints

82b9160

Change naming for strictness type

fb81f7a

Update all_in_range docsting

15ac9f7

Add explicit strictness in tests

0e64342

bpalmeiro mentioned this pull request Jan 14, 2025

ICAROS: City Tools #861

Open

4 tasks

bpalmeiro added 11 commits January 14, 2025 17:55

Cosmetics

769968b

Add match to warns enviroment

582b1a9

Remove unused import

b8a251a

Reword apply_geo_correction docstring

c76c73c

Reword docstings in correction tests

c745cc4

Change function names to imperative

59386dd

Change variable naming for clarity

0c635d0

Update docstrings with right types

61005e3

Remove wordines in given for tests

3bed7d9

Change floats to ints

7d12409

And alignment accordingly

Replace np.sum for np.count_nonzero

8b584bb

bpalmeiro force-pushed the data_preparation branch from 8aeb8c2 to 8b584bb Compare January 14, 2025 17:36

gonzaponte reviewed Jan 22, 2025

View reviewed changes

bpalmeiro added 4 commits January 22, 2025 14:48

Add robust seeds

85f3834

Remove mixing hypothesis and np.random

273ce0b

Include extreme cases

c35d18a

Simplify logic

7b5f2a9

bpalmeiro force-pushed the data_preparation branch from 5d06f7c to 4443df5 Compare January 22, 2025 14:07

bpalmeiro added 2 commits January 22, 2025 22:34

Cosmetics and style

91b69ac

Fix Optionals both for typehings and docstrings

0ac5006

bpalmeiro force-pushed the data_preparation branch from 4443df5 to 0ac5006 Compare January 22, 2025 21:49

gonzaponte reviewed Jan 23, 2025

View reviewed changes

mcidlaso force-pushed the master branch from f0b9a75 to 72df21e Compare September 9, 2025 11:04

		input_mask : Optional[np.ndarray] = None ,
		eff_interval : Optional[Tuple[float, float]] = [0,1] ,

	eff_interval (Optional)
	eff_interval: (float, float), optional

	eff_interval (Optional)
	eff_interval: Tuple[float, float], optional

Conversation

bpalmeiro commented Mar 7, 2024

Uh oh!

jahernando commented Apr 4, 2024

Uh oh!

gonzaponte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gonzaponte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gonzaponte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants