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

Address joss paper comments #103

Merged
merged 120 commits into from
Aug 6, 2024
Merged

Conversation

jschepers
Copy link
Collaborator

@jschepers jschepers commented Aug 6, 2024

Dear @cbrnr,
thanks a lot for your review and the useful suggestions regarding the paper. Thank you also for being patient with us.

Please find our changes and explanations regarding your review suggestions below.

Title: "Toolbox" has strong MATLAB vibes, I suggest to avoid this term and use e.g. "package" instead. My suggestion for an improved title is: "UnfoldSim.jl: Simulating continuous event-based time series data for EEG and beyond"

That's a good idea. We replaced "toolbox" with "package" and adopted the title that you suggested.

Statement of need: "For a multitude of reasons, we often need to simulate such kind of data: Simulated EEG data is necessaryuseful to test preprocessing and analysis tools, ..."

We adapted the sentence as you suggested.

At the end of the "Statement of need" section, you could mention how UnfoldSim.jl overcomes each of these limitations (in a short sentence)? Otherwise, it is not really explicit why being MATLAB-based is a limitation (although most readers will probably know it of course).

We adapted the sentence (+ added a new sentence) in the following way:

" While other EEG simulation toolboxes exist, they each have limitations: they are dominantly based on the proprietary MATLAB software, they do not simulate continuous EEG, and they offer little support for designs more complex than two conditions or with non-linear effects. In contrast, UnfoldSim.jl is free and open-source and it allows to simulate continuous EEG signals even for complex designs."

Figure 1 (upper panel): Parameters in the legend are on a log scale, but the plot shows distributions on a linear scale. This makes it a bit difficult to relate the selected parameters to the distributions. I suggest to convert the parameters in the legend accordingly.

You are right, it's a bit difficult to relate the two. However, we would like to keep the parameters on log scale since this is how we parameterized the distribution in our implementation i.e. the user needs to supply the parameters also on log scale. But to make it clearer that these are two different scales, we added a comment in the figure caption:

"Figure 1: Illustration of the inter-onset distributions. The colours indicate different sets of parameter
values. Please note that for the lognormal distribution, the parameters are defined on a logarithmic scale,
while the distribution is shown on a linear scale.
"

Figure 1 (lower panel): For the uniform distribution, it would be more intuitive to mention the offset before the width in the legend.

Thank you for the suggestion. In principle, we agree but we want to keep the order of the parameters consistent with the other distribution i.e. the lognormal distribution.

Figure 2 A: Why is there a negative offset for ExponentialNoise? I assume the reason is to avoid overlaps, which is a good idea, but then I would add a custom offset to each noise type to show each time series without any overlap.

Thanks for the question. There is no explicit offset for the ExponentialNoise. However, in the current implementation the AR spectrum is defined over the whole signal length and the noise signal varies a lot over time (including a DC offset). We plan to revise the implementation of the ExponentialNoise and make the autocorrelation more local. For now we chose another seed for the visualisation and followed your suggestion to add an offset to all noise types to reduce their overlap (and adapted the figure caption accordingly).

New version: https://github.com/unfoldtoolbox/UnfoldSim.jl/blob/address-joss-paper-comments/joss_paper/plots/noise_types.svg

"Figure 2: Illustration of the different noise types (indicated by colour). Panel A shows the noise over
time. Please note that the noise signals are shifted by 5 μV for visualisation purposes. Panel B displays
its $\text{log}_{\text{10}}\text{(power)}$ at normalized frequencies"

Figure 2 B: Why is NoNoise in the legend (clearly, it is not shown in the PSD)?

Good catch, thank you. We removed NoNoise from the noise samples plot and from the legend.

Figure 2 caption (nitpick): log and power should not be italicized.

Thank you for pointing it out. We fixed it.

Simulation example, step 1: "continuous variable with 10 levels" sounds like you describe a factor. I guess the term "levels" should be replaced here with e.g. "values" or something like that.

We agree. We changed "levels" to "values".

All code snippets: Is it really necessary to suppress output with ;? If not, I'd rather not see this throughout the entire manuscript (you know, MATLAB vibes again)...

You are right, it's not necessary in this context and we removed the ";".

Table 1: Where does "latency" come from and why does it have the specific values shown here?

Thanks for catching this. This was indeed a mistake since latency is only introduced in the final event table (which is created in the simulate call when the onsets have been sampled). We removed it from the table.

Simulation example, step 2: Just wondering, why has the basis to be specified as a function call as opposed to just passing the function?

Thanks for the question! Right now we expect an array, thus the evaluation. Conceptually we reserved passing a function to the case where we want to modify the basis function depending on the design (implemented but not yet merged in #91).

Simulation example, step 2: Maybe relate these specific parameters 5 and 3 to the desired component(s)?

We adapted the sentence in the following way:
"For the first component, we use the prespecified N170 base with an intercept of 5 μV and a condition effect of 3 μV
for the “face/car” condition
i.e. faces will have a more negative signal than cars."

I recommend to shorten the validation part, but if you are going to keep the code snippet, then briefly explain UnfoldModel.

Thank you for the comment. We think the validation part is necessary to visualize the simulation (especially the regression model parts). Without it, it might be difficult for readers to relate their parameter choices with the simulation result. We therefore followed your second suggestion and include a brief mention of Unfold Model in the text. But we refrained from writing a long explanation, as you rightfully point out, this is about simulation and not the model fit.

Final nitpick: please use a ' instead of ´ in "Germany's Excellence Strategy".

Thank you for catching this. We adapted it accordingly.

Regarding your suggestions to shorten the validation part and the package references

As argued above, we did not shorten the validation part for now. We think it is important to visualize the simulation - the alternative would be to simply hide the code, we could do that, but right now we like the transparency offered. We also decided against removing package references. The additional space is negligible and at the end of the paper, where it doesn't distract the reader. On the contrary, we think these citations are actually important for scientists within academia to show that their tools are actively used in research.

Here you can find the newest pdf version of the paper as an Artifact: https://github.com/unfoldtoolbox/UnfoldSim.jl/actions/runs/10251541720

jschepers and others added 30 commits January 9, 2024 10:45
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>
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>
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>
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>
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>
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>
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>
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>
@cbrnr
Copy link

cbrnr commented Aug 6, 2024

Thanks for your detailed response, I'm very happy with the changes, so I think this PR should be merged (so the paper can be re-rendered). Let's continue the review process in openjournals/joss-reviews#6641.

@jschepers
Copy link
Collaborator Author

Thanks a lot for your quick response! I'm happy that you like the changes. Then I'm going to merge the PR now.

@jschepers jschepers merged commit 68c1786 into joss-paper Aug 6, 2024
5 checks passed
@jschepers jschepers deleted the address-joss-paper-comments branch September 26, 2024 11:17
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.

3 participants