-
Notifications
You must be signed in to change notification settings - Fork 7
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
CSD applied to Dansgaard-Oeschger events #48
Conversation
As suggested, we now use DocumenterCictations and the @cite macro. Minor corrections were made to the DO-events example, especially the conclusions. It is still somewhat ugly that the transition times are hand-coded and we might want to make that more elegant in the future. |
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.
Hi Jan, thanks this is great! I am leaving one review, that is small and easy to tackle. I am short on time, so I'll do another review later, but you might as well address this one now if you can.
I agree with all that you said. I also agree that Thus the user only computes one change metric value and therefore p-value per segment. But be sure to state this in the documentation string.
For the example that you have written: start segmenting the code blocks more. Don't have 100-200 l.o.c. in one code block, try to separate them more so that instead of 1 paragraph and then 100 lines of code, there is 1 sentence then 20 loc then another sentence then 20 loc etc.
That's not a fundamental rule or anything. Sometimes its fine to have 100 or 200 locs. But from time to time try to avoid it, especially at the early parts of the tutorial.
I did not understand that. Can you please try to explain again what you said above? Additionally, please point me to the specific lines of source code where you are talking about so that it is easier for me to follow. |
- `t_change`, the time vector of the change metric. | ||
|
||
- [`config::SegmentWindowConfig`](@ref), used for the analysis. | ||
""" |
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.
Should clarify what I tried to convey when describing how the results of a segment analysis are structured!
) | ||
x_change[k, i] = chametric(z) | ||
end | ||
end |
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.
This is the main difference to a sliding window analysis
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.
Alright, seems good. There are some crucial documentation improvements we need to do first.
Besides those, I would argue that we should rename SegmentWindowConfig
to SegmentedWindowConfig
. The window segemenration has already been done by the user.
docs/src/examples/do-events.jl
Outdated
#= | ||
## Hindcast on NGRIP data | ||
|
||
As one can see... there is not much to see so far. Residuals are impossible to simply eye-ball and we therefore use TransitionsInTimeseries.jl to study the evolution, measured by the ridge-regression slope of the residual's variance and lag-1 autocorrelation (AC1) over time. In many examples of the literature, including [boers-early-warning-2018](@cite), the CSD analysis is performed over segments (sometimes only one) of the timeseries, such that a significance value is obtained for each segment. By using `SegmentWindowConfig`, dealing with segments can be easily done in TransitionsInTimeseries.jl and is demonstrated here: |
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.
Please reference the type SegmentationWindowConfig
with the @ref
syntax.
src/analysis/windowed_indicators.jl
Outdated
- [`SlidingWindowConfig`](@ref). | ||
- [`SegmentWindowConfig`](@ref). |
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.
I am unhappy about the documentation strings of these two config structs. If you read them, they are almost entirely identical. It's as if they assume the user already knows what these things do. They don't even discuss the differences between the two.
We need to re-write both docstrings. First, in SlidingWindowConfig
we need to say that this does an analysis of a sliding window over the input timeseries to estimate the indicators, and then a second sliding window analysis over the indicators to estimate the change metrics (i.e., as in the Tutorial@ref
). Then, in SegmentWindowConfing
, we shouldn't copy paste stuff. For all arguments that are the same, say "the arguments x, y, z
and keywords w, o, p
are exaftly like in SlidingWindowConfig@ref
.". Then, describe what is the difference of this type. Say that it does sliding window over pre-selected windows by the user, but then computes only a single change metric for this window, and hence, in conrast to SlidingWindowConfig
, this type does not have any window parmaeters for the change metric.
src/analysis/windowed_indicators.jl
Outdated
t, x, t_indicator, x_indicator, t_change, x_change, config | ||
) | ||
end | ||
|
||
""" | ||
estimate_indicator_changes(config::SegmentWindowConfig, x [,t]) → output |
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.
This function should NOT have two documentation strings. The function does the same thing. It performs the estimation of indicators and change metrics as instructed by the configuration struct. Hence, we only need one simple docstring that shouldn't be more than 2 or 3 lines of text. The main explanation is happening in the configuration structs. The configuration strucs already say that they are passed into estimate_indicator_cjhanges
.
because in the future it might not operate exclusively with windows, e.g., with the PELT algoritm
@JanJereczek I merged this in. Notice that the PR violated the rule "do only 1 thing", as it actually did three things:
However, that's okay for now, because all thse three things were tightly interlinked and we needed the example to see how the new interface could be used. |
This PR mainly provides an example ($\delta^{18}O$ record from NGRIP. This time series displays DO-events and therefore many abrupt transitions in real-world, paleoclimatic data. Furthtermore, the same analysis is performed on simulation data from CLIMBER-X (less noisy and even time sampling), an Earth Model of Intermediate Complexity able to reproduce DO-like events.
docs/src/examples/do-events.jl
) of Critical Slowing Down applied on theRemark: The time steps at which the DO-events are visible in the NGRIP timeseries come from Rasmussen 2014 and should be downloaded in future. For now, hand-coded. In the best case, this should be replaced by an automated procedure like those provided by
ChangePoints.jl
, which however did not give good results so far on the NGRIP data.This PR additionally comes with minor suggestions:
DocumenterCitations.jl
as suggested and addedrefs.bib
.segmented_significance()
was introduced to perform the sliding window analysis such that a single significance value per segment and per indicator is output. This is subject to future changes since I want to introduce a new structGrowingWindowViewer
. We should discuss this live at some point.docs/src/tutorial.md
bydocs/src/examples/tutorial.jl
such that the tutorial is built by Literate.jl.precompute_ridgematrix
was renamed toridgematrix
since it simply computes the matrix.