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

Fix logger interface for histograms #125

Merged
merged 9 commits into from
Jul 25, 2023

Conversation

nomadbl
Copy link
Contributor

@nomadbl nomadbl commented Jun 30, 2023

I noticed that the example for histograms at examples/Histograms.jl was not generating the histogram for histogram/loggerinterface/autobin.
That is, the logging interface for histograms is broken (perhaps because of my previous PR #124 ).

This PR fixes this issue.
In fixing the issue, I made some changes to how histograms are made, in two ways:

  • Histograms are made using fit(Histogram, data, [bins]) instead of calling Histogram(...) which makes the code clearer, more concise and frees the user of the constraint of having bins of a specific length if he wishes to specify them.
  • Previously the inner workings of making the HistogramSummary was pretty convoluted, which also made debugging difficult. There are less functions now and less ways they can be calling each other.

Since I am deleting functions here, we might want to bring them back and mark them as deprecated instead of deleting outright. Please let me know of such cases.

P.S. The hostogram examples themselves contained mistakes, so I fixed that as well.

@nomadbl nomadbl added the bug Something isn't working label Jul 6, 2023
@PhilipVinc
Copy link
Member

Log vector was used to log.. vectors under the histogram interface, but those are technically not histograms.

It was a functionality used by some people so I would not remove it.

@nomadbl
Copy link
Contributor Author

nomadbl commented Jul 8, 2023

Log vector was used to log.. vectors under the histogram interface, but those are technically not histograms.

It was a functionality used by some people so I would not remove it.

Ok I did not realize this. It seemed like keeping log_histogram was enough, and log_vector was a confusing naming which is better removed. This is precisely why I was waiting for a proper review here :)
What then was the intention for log_vector? How were they to be displayed?
If I were to extrapolate here, I'd say it is meant to have bins 0:length(vector) (i.e. length of vector +1 ) and "weights" of the vector values rather than an actual counting of the values in the vector.
If so then I'll add a proper documentation to clarify, and make sure it works as intended.

@nomadbl
Copy link
Contributor Author

nomadbl commented Jul 21, 2023

I brought back log_vector.
What do you think of marking it with @warn "some deprecation warning"? @PhilipVinc @oxinabox
Bar charts are really what people should be using instead for this use case.

@nomadbl
Copy link
Contributor Author

nomadbl commented Jul 25, 2023

For now I am leaving it as is, we can decide on deprecation later.
I also deleted one of the paths for preprocess of Tuple{Array,Array}, since it's better to either auto-bin with Arrays or get a user specified Histogram to log. This also frees up the Tuple{Array,Array} for potential uses we might come up with.
Edit: I saw later in the docs that it was supposed to be deprecated anyway

@nomadbl nomadbl merged commit 3d9c1a5 into JuliaLogging:master Jul 25, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants