Save mu attribue in GRIDE

[diegodoimo]

Add option to save mus also in gride (for consistency with TwoNN)

[codecov-commenter]

Codecov Report

Merging #68 (8c5f559) into main (e32c0e9) will increase coverage by 0.03%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   80.57%   80.61%   +0.03%     
==========================================
  Files          10       10              
  Lines        1138     1145       +7     
==========================================
+ Hits          917      923       +6     
- Misses        221      222       +1     

Impacted Files	Coverage Δ
dadapy/id_estimation.py	82.89% <80.00%> (+0.08%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e32c0e9...8c5f559. Read the comment docs.

[diegodoimo]

flake8 does not pass because the function is too long. There are too many 'if' conditions. I would ignore this estetic aspect for the moment if the rationale of the new attribute is fine for you

[AldoGl]

If we want to ignore it we need to add an exception (see, e.g., here https://flake8.pycqa.org/en/3.1.1/user/ignoring-errors.html). It should be relatively easy, I can do it later if needed

[diegodoimo]

If we want to ignore it we need to add an exception (see, e.g., here https://flake8.pycqa.org/en/3.1.1/user/ignoring-errors.html). It should be relatively easy, I can do it later if needed

I added a helper function to compute maximum likelihood with gride, '_compute_id_gride' which is similar to what we do with _compute_id_2NN. In this manner, we solve the issue

[AldoGl]

Ok great, if it's ok I will also have a look at the code tonight to see if I can give you further feedback

[AldoGl]

This can go for now, but eventually it would be nice to make gride and 2NN more consistent between each other.

In this moment they are not, for instance:

• return_id_scaling_2NN does not have a flag "save_mus" while return_id_scaling_gride does
• compute_id_2NN has a flag called "set_attr" that implicitly also saves the muse, while compute_id_gride does not even exist

I think that eventually (even in another PR) we should make them coherent, meaning that we should create a function compute_id_gride with a "set_attr" argument.

[diegodoimo]

This can go for now, but eventually it would be nice to make gride and 2NN more consistent between each other.

In this moment they are not, for instance:

• return_id_scaling_2NN does not have a flag "save_mus" while return_id_scaling_gride does
• compute_id_2NN has a flag called "set_attr" that implicitly also saves the muse, while compute_id_gride does not even exist

I think that eventually (even in another PR) we should make them coherent, meaning that we should create a function compute_id_gride with a "set_attr" argument.

Yes. But the problem of consistency is not the matter of this PR. These functions have never been consistent so far and for sure sooner or later we should improve the structure of the module. But this was not the purpose of this specific PR

[AldoGl]

Ok, then let's move on, I also approve the PR 👍🏼