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

Restructure package around python. #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JoeZiminski
Copy link

Hey @asoroosh!

This PR restructures the package as a python-focused package and places all Matlab code in a matlab folder. The structure within this folder is the same as it was previously, everything has just moved down one level. But let me know if I've made an oversight and it will mess up the Matlab side of things!

This PR then adds:

  • pyproject.toml that includes installation instructions for pip and pypi. You can do pip install -e . in the top level folder and it will be installed as an editable package. This file also sets a lot of the configs for the linting that will be actually added in a follow up PR. Also check your happy with the the description!
  • 'MANIFEST.injust tellspipwhat to install / upload topypi` when the time comes.
  • The python version now has its own (currently empty) README.md!
  • renamed the python part to dvars the proposed package name.

Of course this is all just suggestions feel free to change / query anything! After this we can introduce the linting.

@asoroosh
Copy link
Owner

asoroosh commented Mar 25, 2024

This is super helpful, @JoeZiminski -- Thanks!

Couple of quick notes;

1 -- The package/paper has two main component; DSE decomposition and inference for DAVRS (also, as you have found, we have a DSEcalc() and a DVARS_calc(). Now the Q is whether we will be calling the package DVARS or DSE or anything else?

2 -- FYI; DSE has three component; D (Fast or DVARS), S (Slow variance) and E (Edge variance) -- something interesting and less explored here is that once the D and S converge (i.e. each explain ~50% of the variance), then we have a pretty 'clean' data set. There is an intrinsic relationship between the autocorrelation and the divergence between D and S which I need to brush my memory on

3 -- Before we go ahead with this PR, would that be best to test the matlab and python version against each other? I am sure I have done these tests previously, but it is been a while and I rather have these tested before we go ahead. A test on simulated data OR a nifti file should do the job. Give me some time to put these together; simulation could actually be better since we can use this in the future for merge/commit testings?

@JoeZiminski
Copy link
Author

Hey @asoroosh good points

  1. My understanding is 'dvars' is a more general procedure introduced earlier (std of difference between scans) and DSE is the decomposition from the paper? In that case maybe DSE makes more sense. For the package name, it could also include the purpose e.g. dse_qualitymetric or dse_denoising (does it perform denoising as well as the bad-volume estimation? apologies if anything incorrect, I am only 1/3 of the way through the paper :D).

  2. This is very cool I will think more about this.

  3. I agree it would be good to test everything as a first step before merging. In Backwards compatibility tests. #9 there is a layout of possible areas to target for testing. Stimulated data is much more flexible and avoids the hassle of having to handle the raw data. That been said using simulated data only will leave some significant parts of the code untested. There is a nice setup with GIN repository to host data and 'pooch' to pull it during testing. A colleague as it setup on github actions such that the raw data for testing is cached so you don't have to download it every time for CI. Maybe we could do something similar? Do you have any test datasets?

The only downside is the package is only tested on one dataset, so simulated data could also be nice complement to stress-test against different data features e.g. high / low autocorrelation

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.

2 participants