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

change verbose::Bool with verbosity::Int #127

Conversation

pasq-cat
Copy link
Member

@pasq-cat pasq-cat commented Oct 22, 2024

as discussed before in #125 .
I storngly suggest to review this pr carefully because the references to verbose were many and not all of them used in the same context.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.84%. Comparing base (37f6bda) to head (9ce642f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   96.48%   95.84%   -0.65%     
==========================================
  Files          22       21       -1     
  Lines         655      578      -77     
==========================================
- Hits          632      554      -78     
- Misses         23       24       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pasq-cat pasq-cat requested a review from pat-alt October 22, 2024 23:17
@pat-alt
Copy link
Member

pat-alt commented Oct 23, 2024

Thanks a lot @pasq-cat, I'll try to have a look this week (please ping me again here/Teams/our next meeting if I should forget 😅 )

@pasq-cat pasq-cat mentioned this pull request Oct 23, 2024
@pasq-cat
Copy link
Member Author

Thanks a lot @pasq-cat, I'll try to have a look this week (please ping me again here/Teams/our next meeting if I should forget 😅 )

Friendly reminder

Copy link
Member

@pat-alt pat-alt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pasq-cat looks good to me, although I'm not really sure what's effectively changed here, i.e. isn't this the same as before with false -> 0 and true -> >0?

Maybe I'm missing something

@pasq-cat
Copy link
Member Author

@pat-alt more or less. It's to avoid verbose = verbosity == 0 ? false : true in the interface. I could have just changed verbose in verbosity but once I was changing it I thought that with a direct support to integers in the future you could add multiple level of verbosity. In fact, Mlj adopts this strategy.

@pasq-cat pasq-cat merged commit 38b6e69 into main Oct 29, 2024
10 checks passed
@pasq-cat pasq-cat deleted the 125-change-the-verbose-param-of-optimize_prior-from-a-boolean-to-an-integer branch October 31, 2024 16:48
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.

change the verbose param of optimize_prior! from a boolean to an integer?
2 participants