Add data preparation functions for the Kr map computation #865

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.

invisible_cities/core/core_functions.py

invisible_cities/core/core_functions_test.py

invisible_cities/reco/icaro_components.py

invisible_cities/reco/icaro_components_test.py

gonzaponte

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

invisible_cities/core/core_functions.py

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

invisible_cities/core/core_functions.py

+        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

invisible_cities/core/core_functions.py

invisible_cities/reco/icaro_components_test.py

And alignment accordingly

gonzaponte

Mostly cosmetics

invisible_cities/reco/icaro_components.py

invisible_cities/reco/icaro_components_test.py

invisible_cities/core/core_functions.py

invisible_cities/reco/icaro_components.py

invisible_cities/reco/icaro_components_test.py

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

invisible_cities/core/core_functions.py

+        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

invisible_cities/core/core_functions.py

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


Suggested change

Returns

Returns

add blank line before Returns

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

invisible_cities/reco/corrections.py

+    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

invisible_cities/reco/corrections.py

+    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

invisible_cities/reco/corrections_test.py

+@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

invisible_cities/reco/icaro_components.py

+                             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

invisible_cities/reco/icaro_components.py

+                          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

invisible_cities/reco/icaro_components.py

+    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

invisible_cities/reco/icaro_components.py

+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

invisible_cities/reco/icaro_components.py

+    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

bpalmeiro added 2 commits December 30, 2024 13:51

Remove hypothesis fof random test

1c0dbd3

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data preparation functions for the Kr map computation #865

Add data preparation functions for the Kr map computation #865

bpalmeiro commented Mar 7, 2024

jahernando commented Apr 4, 2024

gonzaponte left a comment

gonzaponte left a comment

gonzaponte Dec 30, 2024

gonzaponte Jan 23, 2025

gonzaponte left a comment

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

gonzaponte Jan 23, 2025

		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

Add data preparation functions for the Kr map computation #865

Are you sure you want to change the base?

Add data preparation functions for the Kr map computation #865

Conversation

bpalmeiro commented Mar 7, 2024

jahernando commented Apr 4, 2024

gonzaponte left a comment

Choose a reason for hiding this comment

gonzaponte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonzaponte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment