Skip to content

[WIP] Documentation, Test cases and CI for trx_file_memmap - #13

Merged
arokem merged 7 commits into
tee-ar-ex:masterfrom
ValHayot:documentation
Mar 30, 2022
Merged

arokem merged 7 commits into
tee-ar-ex:masterfrom
ValHayot:documentation

Conversation

@ValHayot

Copy link
Copy Markdown
Contributor
  • Extended docstrings for trx_file_memmap.py
  • Added type annotations to trx_file_memmap.py
  • Added test cases [WIP]
  • Created a GitHub workflow for CI

@frheault frheault left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I went through the code and everything looks great.
I tested it with my example code/data and play around a little in a console.
I was just trying to make sure memory and/or hard drive were freed up at the right moment.

And I ran the test, everything seems to be working as intended!

Is it possible to add a requirements.txt with dipy, numpy==1.21 and pytest?
(Maybe writing somewhere this only works in python3.9?

@arokem

arokem commented Dec 1, 2021

Copy link
Copy Markdown
Member

I added a setup.cfg file in ValHayot#1.

I also added a license file. @frheault : are you OK with BSD 2-clause?

@frheault

frheault commented Dec 3, 2021

Copy link
Copy Markdown
Collaborator

I just realized a small error:
In deepcopy there is a conversion in case the user assigns an affine as numpy array and convert it to a list for the json file.
BUT if the affine (and dimensions) are already lists it crashes, a check should be done with isinstance first.

tmp_header["VOXEL_TO_RASMM"] = tmp_header["VOXEL_TO_RASMM"].tolist()
tmp_header["DIMENSIONS"] = tmp_header["DIMENSIONS"].tolist()

@arokem arokem mentioned this pull request Mar 22, 2022
@arokem
arokem merged commit 7ccaf10 into tee-ar-ex:master Mar 30, 2022
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.

4 participants