-
Notifications
You must be signed in to change notification settings - Fork 2
Add time rescaling to inhomogeneous #81
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
84456aa
Add time rescaling to inhomoggeneous
rsenne 8635d07
Fixes to copilot issues
rsenne 6d7814e
Update src/univariate/poisson/inhomogeneous/inhomogeneous_poisson_pro…
rsenne 4bb42d8
Workaround for bug in histroy
rsenne db3aadf
Attempt at JET fix
rsenne 7774064
Add Pkg to requiremtns
rsenne File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,8 @@ using DensityInterface | |
| using Documenter | ||
| using Distributions | ||
| using ForwardDiff | ||
| using JET | ||
| using LinearAlgebra | ||
| using Pkg | ||
| using PointProcesses | ||
| using Random | ||
| using Statistics | ||
|
|
@@ -20,8 +20,15 @@ DocMeta.setdocmeta!(PointProcesses, :DocTestSetup, :(using PointProcesses); recu | |
| Aqua.test_all(PointProcesses; ambiguities=false, deps_compat=(; check_extras=false)) | ||
| end | ||
| @testset verbose = false "Code Linting" begin | ||
| # test on 1.11 at a minimum and not on pre-release | ||
| if VERSION >= v"1.11" && isempty(VERSION.prerelease) | ||
| # Skip JET on Julia pre-releases (where JET typically hasn't caught up | ||
| # yet and there is no compatible JET version to resolve against). JET is | ||
| # not listed in test/Project.toml's [deps] for the same reaso, having | ||
| # it there would make `Pkg.test` fail at the resolution step on | ||
| # prereleases, before this guard ever runs. Install on demand only when | ||
| # we're on a stable Julia. | ||
| if isempty(VERSION.prerelease) | ||
| Pkg.add("JET") | ||
| @eval using JET | ||
|
Comment on lines
+23
to
+31
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great. That was annoying ; ) |
||
| JET.test_package(PointProcesses; target_modules=(PointProcesses,)) | ||
| end | ||
| end | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have to check if the order was changed for new_tmax, then isn't it possible that the order of any pair of events got switched? In this case, should the
Historybe initialized withcheck_argsset totrue?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. My mistake. After reviewing the code of
Historyi think this speaks to an underlying issue with our logic for input handling. I think History might be better of as a validator and not a fixer. What i mean by this is I feel history might be better of only qualifying if the history times meet the proper checks (i.e., bounded properly and sorted).My thinking for this is that there are three reasonable cases unsorted data happens: i.) a user has unsorted data and needs to sort it ii.) a user has an actual error in their data they don't know about iii) we have numerical issues that are introduced from independent quadrature calls.
Right now the behavior of
Historybasically conflates all three as equally valid/equally reasonable occurrences. I don't think we should try to reason about what the case of the above is except when we know for sure what is happening like in numerical issue introduced by quadrature.On our side my thought is we handle these numerical issues and then use
Historyas a way to validate our own code works as expected (e.g., by sorting events and also expanding thetmax). In the case of the user i think we should just throw an error. That way they have to opt-in to any changes of their data and or reciprocally opt-out.This is getting a bit off-topic for this specific case, but I think this is something that might be at the least thinking about more concretely. We could easily supply some easy helpers so that a user knows they are opting into these sort of data fixes e.g.,
sort_history!andrestrict. This would be a breaking change but we aren't at major version 1 yet so semantic versioning doesn't require us to notify users but we could at least add a deprecation cycle if we wanted to.Hope this makes sense--let me know if it doesn't! I can also move this over to a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My answer to the
Historystuff will be in the main thread. But about the ordering, since we will need to call quadrature n+1 times anyway, can't we accumulate the integral of interarrival times instead of calculating the integral of each event time? I would guess quadrature would not return negative results for non negative functions, therefore this would not change the ordering.