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 option to save mus in compute_id_2nn #62

Merged
merged 1 commit into from
Jun 29, 2022
Merged

add option to save mus in compute_id_2nn #62

merged 1 commit into from
Jun 29, 2022

Conversation

diegodoimo
Copy link
Collaborator

I believe that we should give access to the mus. This would allow the user to asses the goodness of the model

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #62 (e246000) into main (a1f83b7) will decrease coverage by 0.05%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
- Coverage   80.61%   80.56%   -0.06%     
==========================================
  Files          10       10              
  Lines        1130     1132       +2     
==========================================
+ Hits          911      912       +1     
- Misses        219      220       +1     
Impacted Files Coverage Δ
dadapy/id_estimation.py 82.79% <50.00%> (-0.31%) ⬇️

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 a1f83b7...e246000. Read the comment docs.

@AldoGl
Copy link
Collaborator

AldoGl commented Jun 29, 2022

Ok I see your point and I am in favour of this addition, however:

  • I don't necessarily agree on having an additional flag. Can't we just save them by default when the set_attr flag is true?

  • Also, can we think of a more informative name than "mus"? The name r2_over_r1_ratio could be an option, but I am very open to other ideas or to keep "mus" if you think is better.

@imacocco what's your opinion on this?

@imacocco
Copy link
Collaborator

I would definitely avoid another flag. Let's stick to set_attr and make the variable available to the user, maybe with the name r2_r1_ratio.

@diegodoimo
Copy link
Collaborator Author

Don't know about the name. We use mus everywhere in code and in papers and has the advantage to be short to type

@diegodoimo
Copy link
Collaborator Author

good for me

@AldoGl
Copy link
Collaborator

AldoGl commented Jun 29, 2022

Wait, I found a couple of problems, I am fixing them now

fixed linting


remove flag form mus; save them as default attribute


fix
@AldoGl AldoGl merged commit 40eb306 into main Jun 29, 2022
@AldoGl AldoGl deleted the add-save_mus branch June 29, 2022 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants