Improve UnfoldSim Docstrings

[jschepers]

Here we can list Doc strings that need to be revised either because they are unclear or incomplete or because their formatting is not correct or inconsistent. Afterwards, we can also have a look at the code to see whether any doc strings are missing for exported functions.

[jschepers]

@behinger Could you please add the missing information for the following docstrings

bases.jl

• PuRF function: Please

  • improve the function description
  • add descriptions for the keyword arguments and the return value
  • and maybe (if you see fit) add second example or adapt existing one
  • decide whether the second signature PuRF(t, n, tmax) should be included or if it's only for internal use and should not be visible

• hrf function: Please

  • decide whether function signature is okay like this or too cluttered and should rather contain only a <keyword arguments> place holder?
  • improve the function description (and format it correctly i.e. one sentence description and a second paragraph if needed.)
  • add descriptions for the keyword arguments and the return value
  • if you like change the example

[jschepers]

@behinger Could you please have a look at/adapt the following docstrings:

component.jl

• MixedModelComponent: " basis::Any: an object, if accessed, provides a 'basis function', e.g. hanning(40), this defines the response at a single event. It will be weighted by the model prediction." -> Is it misleading since a vector is expected? Do we want to change that in the future? (Also for LinearModelComponent)

• Please double-check whether the left-hand side of the formula is ignored for the MixedModelComponent but must be zero for the LinearModelComponent.

• MutlichannelComponent: Revise projection description + add output dimensions. Especially the problem with the two different projection types. Should the pair one also be in the field section or is it confusing?

• simulate_component(rng, c::AbstractComponent, simulation::Simulation) : Clarify docstring

• Check Descriptions of the return value for the simulate_component functions for all component types (Include output type and dimensions)

• simulate_component(rng, c::MixedModelComponent, design::AbstractDesign): revise/clarify docstring; I don't completely understand the part about removing the "normal-noise" from MixedModel. Why is it there and why do we have to remove it? Which code-stub is meant here?

• simulate_component(rng, c::LinearModelComponent, design::AbstractDesign)_ Is it appropriate to say that the return type is Matrix? Is it always Matrix{Float64} or could it also be another type? Is the description of the output dimensions correct?

• weight_σs(σs::Dict, b_σs::Float64, σ_lmm::Float64): Unfortunately, I don't really understand the docstring. Could you please revise/clarify it and add the missing docstring sections.

• simulate_responses(rng, components::Vector{<:AbstractComponent}, simulation::Simulation): Check Returns section (especially output dimensions) and adapt it if it is not correct.

[behinger]

Please double-check whether the left-hand side of the formula is ignored for the MixedModelComponent but must be zero for the LinearModelComponent

We could change this and replace the lhs also with :df_tmp like in MixedModelsComponent then it would be consistentI don't see the downside, except :df_tmp being not useable as a column in events

[behinger]

simulate_component(rng, c::LinearModelComponent, design::AbstractDesign)_ Is it appropriate to say that the return type is Matrix? Is it always Matrix{Float64} or could it also be another type? Is the description of the output dimensions correct?

This was actually surprisingly hard to find out. Turns out we use a ModelFrame in the background. It is theoretically possible by reimplementing a bit to return also other types, but Matrix{Float64} is the default as returned from StatsModels

[behinger]

weight_sigmas - Here it is difficult to provide an example, I think this is such a implementation specific internal function, that we don't need it.

I think in general only the outward facing functions need proper docstrings, for the others it is a nice to have

[jschepers]

Please double-check whether the left-hand side of the formula is ignored for the MixedModelComponent but must be zero for the LinearModelComponent

We could change this and replace the lhs also with :df_tmp like in MixedModelsComponent then it would be consistentI don't see the downside, except :df_tmp being not useable as a column in events

Shall we change this now or shall I add it to issue #125 where I started collecting ideas for minor code revisions?

[jschepers]

@behinger Please have a look at the following points/docstrings:

design.jl

• Where to document the rng use (no deep copy) for RepeatDesign? In RepeatDesign or the corresponding generate_events function or both? For now I just added it to both.
• I added the following note in the RepeatDesign docstring and in the generate_events docstring: "Please note that when using an event_order_function(e.g. shuffle) in a RepeatDesign, the corresponding RNG is shared across repetitions and not deep-copied for each repetition. As a result, the order of events will differ for each repetition."
  • Is it understandable? If not please revise it.
  • Should I just put it in the description paragraph as a normal sentence or in a "warning" box?
• Is the description for the length(design::AbstractDesign) function correct? "Length is the product of all dimensions and equals the number of events in the corresponding events dataframe." If not please adapt it.
• As discussed on Zulip, I added a function function generate_events end to do a general docstring for the generate_events function. Is this/the docstring how you thought it should be?
• Please check and potentially revise the description of the return value for generate_events: "DataFrame: Each row corresponds to one combination of condition/covariate levels which is often equivalent to one stimulus or trial."

[behinger]

• Docboth is good. I read somehwere that documentation can be repeating
• warning I don't think a warning is warranted - only for us, because it is a breaking change ;-) I think someone new to the toolbox with a fresh eye wouldnt be surprised one way or the other.
• length - yes I think that is correct

I checked them, and everything looks tip-top!

[jschepers]

@behinger Please have a look at the following questions/docstrings (some of them do not only concern documentation but also implementation):

headmodel.jl

• Please check docstrings for correctness: Hartmut, leadfield, orientation
• Can I assume that the return type for leadfield(hart::Hartmut; type = "cortical") will always be Array{Float64, 3} and Matrix{Float64} for orientation(hart::Hartmut; type = "cortical")?
• Do the leadfield values or orientation values have a unit? And what's the range of possible values?
• Could you improve the description/explanation of leadfield and orientation?
• In the leadfield example it printed a lot of output. Is this okay or shall I either only print the size of the Array or try to shorten it?
• The current magnitude docstrings are not completely clear to me. Could you please try to clarify and add the missing information? For example, the following points are unclear to me:
  • How does magnitude(headmodel::AbstractHeadmodel) use the orientation specified in the headmodel when it only calls magnitude(leadfiled(headmodel)) i.e. does not include the orientation?
  • I also do not completely understand the explanation of the fallback case using the norm. Why is the fallback case the norm? Which norm is used and why is this equal to the maximal projection?
  • For me, the type keyword argument of the magnitude(headmodel::Hartmut; type = "perpendicular") is not completely clear:
    • If I understand correctly, perpendicular means using the source point orientations provided by the Hartmut model. And it's called perpendicular because the Hartmut model assumes that the sources are oriented perpendicular to the surface of the brain surface?
    • Which other types could you imagine?
  • Do I see it correctly, that only for the magnitude methods which directly take the leadfield as an input, it is possible to get the magnitude also for artefactual (instead of cortical) sources?
  • For the magnitude(lf::AbstractArray{T,3}, orientation::AbstractArray{T,2}) where {T<:Real} method I think it would be worth adding that the dimensions of the orientations (in particular number of sources) have to match those of the leadfield. Potentially also using an assert statement.
  • I only added a Returns section for magnitude(headmodel::AbstractHeadmodel) since this is the most general method and I assume the output description does not change for the other methods. If it does, please also add a Returns section to the docstrings of the other docstrings.

[behinger]

Can I assume that the return type for leadfield(hart::Hartmut; type = "cortical") will always be Array{Float64, 3} and Matrix{Float64} for orientation(hart::Hartmut; type = "cortical")?

I would say no / yes. In principle a Leadfield could be 2D if only calculated for e.g. perpendicular orientation

Do the leadfield values or orientation values have a unit? And what's the range of possible values?

The orientations dont have units, they are typically unit basis vectors, so their norm is 1 and their value between -1 & 1.

The leadfield does have values - but I dont know them, something like V/square-Ampere or something, I dont think it is important to specify right now.

[behinger]

• leadfield example, we dont need that many matrix outputs, just show less of them, e.g. by making the size of your repl much smaller ;-).

I noticed you wrote a docstring for the specific case of Hartmut, whcih is fine, it is the only one implemented. In that case Array x 3 is ok - but I guess there could be a general case with a different headmodel that returns Array x 2 as indicated above. Let's keep the docstring as it is specific for that case, and later in the general case - only if we ever implement a different headmodel ;-) - we update it

[behinger]

• magnitude(headmodel::AbstractHeadmodel) = magnitude(leadfield(headmodel)) this is only for AbstractHeadmodel, not for Hartmut which later get's their own magnitude function magnitude(headmodel::Hartmut; type = "perpendicular")

For me, the type keyword argument of the magnitude(headmodel::Hartmut; type = "perpendicular") is not completely clear:
If I understand correctly, perpendicular means using the source point orientations provided by the Hartmut model. And it's called perpendicular because the Hartmut model assumes that the sources are oriented perpendicular to the surface of the brain surface?
Which other types could you imagine?

Yes - I think there are multiple ways to calculate the which orientation to use. I'm thinking of SVD/largest eigenvalue, PCA, monte-carlo, norm, perpendicular, maybe others. Only two are implemented - I dont think we should invist much time here, but I did invest a little bit to visualize some options:

Maybe this makes it a bit more clear

[behinger]

Do I see it correctly, that only for the magnitude methods which directly take the leadfield as an input, it is possible to get the magnitude also for artefactual (instead of cortical) sources?

indeed, we would need to loop kwargs to leadfield

[behinger]

For the magnitude(lf::AbstractArray{T,3}, orientation::AbstractArray{T,2}) where {T<:Real} method I think it would be worth adding that the dimensions of the orientations (in particular number of sources) have to match those of the leadfield. Potentially also using an assert statement.

sure go ahead!

[jschepers]

Can I assume that the return type for leadfield(hart::Hartmut; type = "cortical") will always be Array{Float64, 3} and Matrix{Float64} for orientation(hart::Hartmut; type = "cortical")?

I would say no / yes. In principle a Leadfield could be 2D if only calculated for e.g. perpendicular orientation

Do the leadfield values or orientation values have a unit? And what's the range of possible values?

The orientations dont have units, they are typically unit basis vectors, so their norm is 1 and their value between -1 & 1.

The leadfield does have values - but I dont know them, something like V/square-Ampere or something, I dont think it is important to specify right now.

Thanks, I added the following line to the orientation docstring: "The norm of the orientation vectors is 1 and the values are between -1 and 1.".

[jschepers]

@behinger Could you please have a look at the following points/questions?

noise.jl

• Is noiselevel a Float64 or and Int or could it also be something else?
• Please complete the field description for imfilter in the WhiteNoise docstring. For now I put the sentence "Using imfilter > 0 it is possible to smooth the noise using Image.imfilter." in the type description but feel free to move it to the field description.
• It's not really clear to me what exactly nu does in the ExponentialNoise definition. So far I only copied your comment from the code for the description. But since we are going to replace this function with a different implementation I guess it's fine.
• Can I assume for simulate_noise that the returned noise is always of type Vector{Float64}?
• To avoid too much redundancy I created a general docstring for simulate_noise (by defining a general method with function simulate_noise end as we did in the case of the experimental design) and removed the docstrings for the different simulate_noise methods. However, for the example I used one specific noise type. I hope that's okay. Please let me know if you have a different idea how to best do this.

[behinger]

• noiselevel is numeric, probably real, in 99% of cases Float or Int, anything that can be multiplied with a vector

Using imfilter > 0 it is possible to smooth the noise using Image.imfilter with a Gaussian kernel with \sigma = imfilter.

• The default should be imfilter = nothing though, right now we always filter but with a gaussian of std = 0.
• Yeah let's leave out exponential noise, I recoded it with a new algorithm anyway
• simulate_noise, why restrict to Float64? Vector{<:Numeric} maybe, or just write it returns a vector? If someone wants Float32 simulations, why not 🤷
• simulate_noise together is good imho. We could add a hint to look at the individual noisetypes for documentation and we could link the useful subtypes(AbstractNoise) for discoverability

[jschepers]

• Should I put any restrictions for noiselevel e.g. noiselevel::Real = 1 or shall I leave it as it is i.e. noiselevel = 1?
• Do you want me to replace the default value of imfilter which is currently 0 with nothing?
• I removed the Float64 from the Returns section of simulate_noise such that it only says that it returns a vector.
• I added the following lines to the simulate_noise docstring: "For details, see the documentation of the individual noise types. Use subtypes(AbstractNoise) for a list of the implemented noise types."

[behinger]

• I would just leave it as is, but if you touch it, you can add it
• yes, nothing for imfilter. it's not breaking though
  The others seem great!