Skip to content

Conversation

@ReneSkukies
Copy link
Member

Lower truncation might be useful to simulate specific experiments where there was/ is an exclusion criterion, artificially changing the distribution.

E.g. in our Oddball experiment, we set a lower RT threshold, throwing out every trial below.

Change log-normal onsets to also include lower truncate
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (17f3fe8) 72.82% compared to head (1e02814) 69.93%.

❗ Current head 1e02814 differs from pull request most recent head 69902f8. Consider uploading reports for the commit 69902f8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
- Coverage   72.82%   69.93%   -2.90%     
==========================================
  Files          10       10              
  Lines         357      316      -41     
==========================================
- Hits          260      221      -39     
+ Misses         97       95       -2     

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

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@behinger
Copy link
Member

hey! great idea!

  • Could you add a unit-test?
  • Could you modify the tutorial to include this option (or at least mention it?)
  • Could you elaborate whether this allows for upper & lower truncation at the same time?

@ReneSkukies
Copy link
Member Author

Can do the first two.
To the last: It does! Didn't check with UnfoldSim, but with the Distribution package. If you have a truncated distribution type, the function just adds the other truncation as well. I.e. a Truncated(LogNormal{Float64}(μ=5.0, σ=1.0); upper=10.0) becomes Truncated(LogNormal{Float64}(μ=5.0, σ=1.0); lower=1.0, upper=10.0) if you apply the truncated function a second time (with a lower bound)

This tests both upper and lower truncation in one go. I think it's fine like this but could also be tested separately in addition.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
ReneSkukies and others added 3 commits December 19, 2023 15:23
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

# Example:
onset_lognormal = LogNormalOnset(; μ = 3, σ = 0.25, offset = 0, truncate_upper = nothing);
onset_lognormal = LogNormalOnset(; μ = 3, σ = 0.25, offset = 0, truncate_upper = nothing, truncate_lower = nothing);
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
onset_lognormal = LogNormalOnset(; μ = 3, σ = 0.25, offset = 0, truncate_upper = nothing, truncate_lower = nothing);
onset_lognormal = LogNormalOnset(;
μ = 3,
σ = 0.25,
offset = 0,
truncate_upper = nothing,
truncate_lower = nothing,
);

Copy link
Member Author

Choose a reason for hiding this comment

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

As this is for the docs only anyway I would omit

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@ReneSkukies
Copy link
Member Author

hey! great idea!

* Could you add a unit-test?

* Could you modify the tutorial to include this option (or at least mention it?)

* Could you elaborate whether this allows for upper & lower truncation at the same time?

Everything implemented now. I only mentioned the truncate lower in the docs because I didn't want to prolong the parameter plots even further. If you still want me to do that I can though.

@jschepers jschepers changed the title include lower truncation for normal distrubution include lower truncation for lognormal distrubution Nov 13, 2025
@jschepers
Copy link
Collaborator

Updated onset type reference page figure:
grafik

@jschepers
Copy link
Collaborator

jschepers commented Nov 13, 2025

I updated the PR in the following way:

  • I adapted the reference page about noise types to include the parameter truncate_lower
  • I added truncate_lower also to LogNormalOnsetFormula
  • I added a truncate_lower parameter value in the existing LogNormalOnsetFormula test and adapted the test output accordingly
  • I added a test to test whether truncating twice with one bound each is the same as truncating once with two bounds

For the last point, I needed to add the Distributions.jl package to the test environment in order to have access to its LogNormal function.
@behinger Do you think this is a bad idea since it adds a new dependency to the test environment? Then I could revert the changes.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@jschepers
Copy link
Collaborator

Apart from the point above, we can merge, in my opinion. Any objections, @behinger oder @ReneSkukies?

@ReneSkukies
Copy link
Member Author

ReneSkukies commented Nov 14, 2025

I added a test to test whether truncating twice with one bound each is the same as truncating once with two bounds

Could you explain to me when that would be an issue? I think I don't really understand (also it's been a while since I had a look at the truncate stuff)

Other than that, I think it's fine. Regarding the Distributions.jl as dependency in test, I am not sure why it would be a bad idea in any case, but isn't Distributions.jl a dep of UnfoldSim anyway? Since I think you only use the package for two or so function calls, you could've done something like UnfoldSim.Distributions.LogNormal (a bit ugly, though). But maybe Julia is smart enough to realize that Distributions is pre-compiled through UnfoldSim anyway and kinda just re-exports it and it wouldn't make a difference? Maybe Bene knows more...

@jschepers
Copy link
Collaborator

Sure, it's basically what Bene asked above and that you already tested a while ago (#57 (comment)).

The way you implemented the lower truncation is after potentially doing an upper truncation (using the truncated function) to do a lower truncation (again using the truncated function on the already truncated distribution).

UnfoldSim.jl/src/onset.jl

Lines 173 to 178 in eadcbfb

fun = LogNormal(onset.μ, onset.σ)
if !isnothing(onset.truncate_upper)
fun = truncated(fun; upper = onset.truncate_upper)
end
if !isnothing(onset.truncate_lower)
fun = truncated(fun; lower = onset.truncate_lower)

And the idea was to make sure that this does not give a different result than doing both truncations at the same time in one call to truncated.

I talked to Bene about your second point, and this would indeed be possible, but it's also fine to have it as a dependency in the test environment, therefore, I will leave it that way.

@jschepers jschepers merged commit 66a7b66 into main Nov 14, 2025
4 checks passed
@jschepers jschepers deleted the truncate_lower branch November 14, 2025 11:56
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.

4 participants