Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add kr evolution functions #894

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from
Draft

Add kr evolution functions #894

wants to merge 27 commits into from

Conversation

carhc
Copy link
Collaborator

@carhc carhc commented Sep 27, 2024

This PR provides the functions for the parameter evolution calculation. It should be the last one of the related PR (#872, #876...) to be approved since it sits on top of them. Test Suit will fail at the beggining. Function tests to be commited in the following days.

@carhc carhc added the testing label Sep 27, 2024
@carhc carhc marked this pull request as draft September 27, 2024 08:06
@jahernando
Copy link
Collaborator

The sigmoid function maybe should be in the module with other functions, or import from scipy.

@jahernando
Copy link
Collaborator

gaus_sig_function Not clear what are x and y, and how yoy compute the seed. Please add comments

@jahernando
Copy link
Collaborator

resolution add comments that you return the relative resolution and error

@jahernando
Copy link
Collaborator

get_number_of_time_bins this seems to me overcoding, when this function, if only used one in the code, can be resumed in just one line of code easy to read!

@jahernando
Copy link
Collaborator

get_time_series_df the comment is not clear, you say that you devide the event but in reality you divide the dst in masks with consecutive time intervals. Please clarify the comment on the definition of the function.

@jahernando
Copy link
Collaborator

In compute_drift_v, please x-check is there is still no access to the DB!

@jahernando
Copy link
Collaborator

e0_xy_correction function, if you use only one time or in one place, this function is maybe over-coding, as it is just very fre lines, but anyhow, it is acceptable.

@jahernando
Copy link
Collaborator

In * computing_kr_parameters* as we discussed this morning, there is no need for the norm_strat and norm_value arguments.

@jahernando
Copy link
Collaborator

cut_effs_evolution as we discussed in the meeting, it should a function, than from the dst and a list of masks returns a list of efficiencies. The computing_kr_parameter functions should call this function to obtain the S1, S2 and Band efficiencies and then store then in the dst and not update them.

@jahernando
Copy link
Collaborator

  • add_krevol* should take as input onloy the configuration parameters, from those you obtain the parameters that configures the internal function add_krevol that does not need to be passed to the arguments of the function.

The internal function add_krevol should take as arguments, if any, kdst, map, and masks (a list of masks). Then the function should:

  1. apply the masks to the dst
  2. get the time range (without the function get_number_of_time_bins
  3. to the function get_time_series_df you can pass the filtered dst *dstf * insteas of dst and then you can remove line 580
  4. remove cut_effs_evolution that now it should be internal to compute_kr_parameters

The comments of what does this function are not right. Please indicate that the function, applies the mask to the dst, split the dst in time intervals, and for each one computes a set of variables, for example life-time, drift-velocity, S1, S2 averaget, etc and returns them in a DataFrame.

@bpalmeiro bpalmeiro linked an issue Oct 18, 2024 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICAROS: parameter evolution
2 participants