Skip to content

Integrating code into bestie template #3

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

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

Conversation

Till223
Copy link
Collaborator

@Till223 Till223 commented Feb 3, 2025

Related issues

Closes #

Checklist

  • I am following the contributing guidelines
  • Tests are passing
  • Lint workflow is passing
  • Docs were updated and workflow is passing

Copy link

codecov bot commented Feb 9, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

version = "0.1.0"

[deps]
Copy link
Member

Choose a reason for hiding this comment

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

There are several dependencies here that are unlikely to be part of the package in the end. These are e.g. CairoMakie, UnfoldSim, or Revise.

I understand that these are needed for package development, but ultimately they are too big of a dependence to be included. You could instead have a playground/ dev environment and then "dev UnfoldRIDE" to your local path in that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've switched to using the /test/Project.toml for now, but I'll think about making a dedicated dev environment. It sounds like a good idea.
The main Project.toml should be clean now.

Copy link
Member

Choose a reason for hiding this comment

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

I dont see it yet, forgot to push?

version = "0.1.0"

[deps]
Copy link
Member

Choose a reason for hiding this comment

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

I dont see it yet, forgot to push?

Project.toml Outdated
Distributions = "0.25.117"
FFTW = "1.8.1"
Parameters = "0.12.3"
Peaks = "0.5.3"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Peaks = "0.5.3"
Peaks = "0.5"

don't fix on bugfix versions, but feature / major versions

Copy link
Member

@ReneSkukies ReneSkukies left a comment

Choose a reason for hiding this comment

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

I am not sure if you need to address everything, and some stuff is for later review, but a bit of work still there.

One of the most important things for now are the documentation and the docstrings. This can be a different PR though, and can be done after you finish writing the thesis.


abstract type RideModus end
struct RideOriginal <: RideModus end
struct RideUnfold <: RideModus end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
struct RideUnfold <: RideModus end
struct UnfoldRide <: RideModus end

For consistency, I would probably keep this the same as in Unfold.jl (where the types are names e.g. UnfoldLineraModel)

But then you'd have to rename RideOriginal to OriginalRide as well I guess

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed them to ModusRide, OriginalRide and UnfoldRide

Copy link
Member

Choose a reason for hiding this comment

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

can I propose AbstractRIDE, UnfoldRIDE and ClassicRIDE / OriginalRIDE?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Bene, that would make it more clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, that works. I think I like ClassicRIDE more than OriginalRIDE.

@debug "Running RIDE algorithm with cfg: $cfg"
## data_preparation
data_reshaped = reshape(data, (1, :))
evts_s = @subset(evts, :event .== 'S')
Copy link
Member

Choose a reason for hiding this comment

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

This should be more general. You usually can't assume people call their events S or R. Especially given the char type.

Copy link
Member

Choose a reason for hiding this comment

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

It's ok for the initial push though. Can be refactored later

@@ -0,0 +1,158 @@
function ride_algorithm(Modus::Type{RideUnfold}, data, evts, cfg::RideConfig)
Copy link
Member

Choose a reason for hiding this comment

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

It's good practice to directly write some docstrings with the function.

@@ -0,0 +1,232 @@
function ride_algorithm(Modus::Type{RideOriginal}, data, evts, cfg::RideConfig)
Copy link
Member

Choose a reason for hiding this comment

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

Please provide docstrings with the functions; for exported functions like here use:
https://unfoldtoolbox.github.io/UnfoldSim.jl/previews/PR138/developer_docs/#Docstring-templates

Copy link
Member

Choose a reason for hiding this comment

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

Pleas comment on what is happening

Copy link
Member

@ReneSkukies ReneSkukies left a comment

Choose a reason for hiding this comment

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

Some more comments and then we can merge ;)
And then tackle documentation 🐙

Project.toml Outdated
Revise = "3.7.2"
StableRNGs = "1.0.2"
Statistics = "1.11.1"
CairoMakie = "0.13"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need a compat for CairoMakie if it's not part of you package ;)

abstract type RideModus end
struct RideOriginal <: RideModus end
struct RideUnfold <: RideModus end
abstract type ModusRide end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
abstract type ModusRide end
abstract type AbstractRide end

As Bene suggested I would rename this

Comment on lines 20 to 21
struct OriginalRide <: ModusRide end
struct UnfoldRide <: ModusRide end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
struct OriginalRide <: ModusRide end
struct UnfoldRide <: ModusRide end
struct OriginalRide <: AbstractRide end
struct UnfoldRide <: AbstractRide end

added function to run the classic version one time for every channel
modified the unfold version to be able to work with multiple channels as input
ClassicRIDE just runs multiple times, once for each channel, then outputs a Vector of RideResults
UnfoldRIDE adapted to handle multiple channels throughout the algorithm
wrote tutorial code for data simulation
…the size of one epoch

changed output C_latencies to present latencies from stimulus onset instead of latencies from epoch onset
adjusted plotting to work with new outputs
changed UnfoldModeRIDE and ClassicRIDE parameters to UnfoldMode and ClassicMode and AbstractMode
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