JuliaStats
Fix #1870: Rename ValueSupport subtypes for improved clarity
Codecov Report
Attention: Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.
Project coverage is 85.99%. Comparing base (
b356da0) to head (fd4da93).
| Files | Patch % | Lines |
|---|---|---|
| src/mixtures/mixturemodel.jl | 66.66% | 1 Missing :warning: |
| src/truncated/normal.jl | 83.33% | 1 Missing :warning: |
| src/univariate/discrete/negativebinomial.jl | 0.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #1872 +/- ##
=======================================
Coverage 85.99% 85.99%
=======================================
Files 144 144
Lines 8666 8666
=======================================
Hits 7452 7452
Misses 1214 1214
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
As mentioned in #1870, I'm not sure yet if it's worth to make this change.
The names Continuous and Discrete are too inclusive in meaning. There are other packages like DataScienceTraits.jl where the word Continuous is used to refer to Continuous variables in general. In this package the meaning is that of ContinuousSupport. This PR uses these more specific names.
Alternatively, I can start another PR that doesn't export Continuous nor Discrete to avoid similar conflicts.
Em sáb., 15 de jun. de 2024, 16:39, David Widmann @.***> escreveu:
As mentioned in #1870 https://github.com/JuliaStats/Distributions.jl/issues/1870, I'm not sure yet if it's worth to make this change.
— Reply to this email directly, view it on GitHub https://github.com/JuliaStats/Distributions.jl/pull/1872#issuecomment-2170593878, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3KOCLSISGMJ4IFGGVLZHSJ7HAVCNFSM6AAAAABJL2OUASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZQGU4TGOBXHA . You are receiving this because you authored the thread.Message ID: @.***>
It's clearly a quite general term - but IMO that's not a problem per se. I think the existence of the same (exported) name in some other package is not a sufficient reason for such a major change in Distributions that probably also causes multiple deprecation warnings in downstream packages (in particular since it seems its use in Distributions predates the one in DataScienceTraits).
Is there a problem in not exporting such general names? I don't think users of Distributions.jl rely on these. And if they do, they can be explicit about it with an explicit import.
Em sáb., 15 de jun. de 2024, 17:51, David Widmann @.***> escreveu:
It's clearly a quite general term - but IMO that's not a problem per se. I think the existence of the same (exported) name in some other package is not a sufficient reason for such a major change in Distributions that probably also causes multiple deprecation warnings in downstream packages (in particular since it seems its use in Distributions predates the one in DataScienceTraits).
— Reply to this email directly, view it on GitHub https://github.com/JuliaStats/Distributions.jl/pull/1872#issuecomment-2170708331, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3PCLSMPLYQKPMJJBSTZHSSLZAVCNFSM6AAAAABJL2OUASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZQG4YDQMZTGE . You are receiving this because you authored the thread.Message ID: @.***>
@devmotion are you willing to unexport the Continuous and Discrete generic names? I can close this PR and open a new one if that is the case.
We can't unexport them in a non-breaking release and there are no plans for a breaking release in the near future.
there are no plans for a breaking release in the near future.
Because no one is actively leading progress in Distributions.jl or because there is a consensus among the active maintainers that breaking releases should only happen after months?
Because my impression is that downstream developers are annoyed by breaking releases of such a core part of the ecosystem (IIRC there are issues with complaints and last time we did one there were also not too many positive reactions on Slack), hence we try to minimize the number of breaking releases, and there's already a list of important points to change/improve in the next breaking release.
Because my impression is that downstream developers are annoyed by breaking releases of such a core part of the ecosystem (IIRC there are issues with complaints and last time we did one there were also not too many positive reactions on Slack), hence we try to minimize the number of breaking releases, and there's already a list of important points to change/improve in the next breaking release.
Isn't that always the case? Why shouldn't we fix the issue at hand in the next breaking release? It is breaking, and nothing is required downstream if we provide a deprecation warning, and document the breaking change properly.
Is there a plan somewhere of the next breaking release? Can you please share it?
Why shouldn't we fix the issue at hand in the next breaking release?
I'm not against changing it in the next breaking release (I still think that it's somewhat questionable to call it a fix) but I'm opposed to making a breaking release where this is the only (?) breaking change and all the breaking changes that have been discussed (more or less publicly) for a possible future breaking release are not addressed.
I also found the issue again that I had in mind: https://github.com/JuliaStats/Distributions.jl/issues/1317
I'm opposed to making a breaking release where this is the only (?) breaking change
But I did not ask for that. What I want to know is which of the two PRs proposed is preferred for the next breaking release. This one (my preferred choice), or a new one that simply unexports the generic names?
@devmotion what is the preferred solution to this problem? What else can we do to help you with the downstream demands? I checked JuliaHub, and literally 0 open-source packages make use of Distributions.Continuous or Distributions.Discrete.
What is the current practice of this dichotomy? I don’t trust the docs very much, I assume it’s a mixture between the eltype of the samples and other notions of „support“ and „discrete“ and „continuous“
@mschauer I understand that it refers to the support of the CDF of the random variable. In any case, the exact meaning here shouldn't affect the issue at hand: the generic Continuous and Discrete names are exported and conflict with other modeling packages that use the words in the more general sense (e.g., continuous variable, not necessarily random).
what is the preferred solution to this problem?
My preferred solution at the time being is to not do anything - there doesn't seem to be an obvious bug to me, the names have been exported for a long time without issue, the problem can be avoided easily on the user side by only loading desired functionality (e.g., using using Distribution: ... or import Distributions), it's unclear how many users are actually affected by inconveniences because other packages started to export the same names, and probably a breaking change wouldn't make it into a new release in the near future anyway.