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

Scalars and properties #3

Closed
nilgoyette opened this issue Jul 10, 2018 · 2 comments
Closed

Scalars and properties #3

nilgoyette opened this issue Jul 10, 2018 · 2 comments

Comments

@nilgoyette
Copy link
Collaborator

"s/p" means "scalars and/or properties"

Premises

  • s/p reading is not so complicated. In fact, it's already working. They are currently saved in the header.
  • s/p shouldn't be in the header. Their names fit there because it's actual header data, but the s/p themselves? I strongly doubt it.

Read all

People calling read_all wants everything in one shot and they don't care about memory consumption. Hence we can change the definition of read_all to

pub fn read_all(
    &mut self,
    bool return_all
) -> (Streamlines, Scalars, Properties) {

So, always return (streamlines, vec![], vec![]) when return_all=false. An empty vec![] doesn't allocate. The reading time should mostly stay the same when false. The usage stays pleasant

let (streamlines, _, _) = writer.read_all(false);
let (streamlines, scalars, properties) = writer.read_all(true);

Of course we modify write_all to receive those 3 arguments. They really should fit!

Iteration

NiBabel::TractogramItem and iteration make sense to me. In NiBabel, they return a struct on iteration, something like

struct TractogramItem {
    streamline, scalars, properties
}

for item in streamlines {
    // Read only streamlines, or zip, or ?
    for p in item.streamline { ... }
}

I wonder though if we should offer 2 iterators.

  • A fast one. The one we already have. It ignores all s/p and only returns the points.
  • A slow but complete one.

I'm not sure about the naming. Maybe we should rename the fast one to iter_points() and keep the standard iter for the complete one? If we only offer one, it should be the complete one, of course.

Usage

s/p are f32. We can't avoid that and it's in fact ok. It's less fun for "grouped" s/p like colors. We should offer a tool to manage colors in a non-terrible way. Nothing complicated, just a method that try to find which properties are the color then returns an iterator of tuple(r, g, b). I want something like

for (p, color, torsion) in streamline.iter()
                           .zip(scalars.colors())
                           .zip(scalars["torsion"]) { ... }

Saving on iteration

We currently have 3 write methods: write_all (already been discussed), write and write_from_iter. They both write a single streamline. We can either change their signatures to receive 2 Option<> OR double them to have a "points" version and a "points and s/p" version.

Here's a read + add colors example that will crash if there's already one or more s/p

let reader = Reader::new("full_brain.trk");
let mut header = reader.header.clone();
header.add_scalar("color_x");
header.add_scalar("color_y");
header.add_scalar("color_z");

let mut writer = Writer::new("copy.trk", Some(header));
for item in streamlines {
    let nb_points = item.streamline.len();
    let mut r = Vec::with_capacity(nb_points);
    let mut g = Vec::with_capacity(nb_points);
    let mut b = Vec::with_capacity(nb_points);
    ... Fill r, g and b with the coloring method you want ...
    
    writer.write(item.streamline, vec![r, g, b], vec![]);
}

TBH, I'm not too sure about the write_from_iter. Maybe it should stay exactly as it is. Maybe not. I'll probably have an opinion when I code it or when I use it.

@nilgoyette
Copy link
Collaborator Author

I now realize that a part of my design is impossible! Namely the "iteration" part. I forgot that

pub type Streamlines = ArraySequence<Point>;

So I can't have a specific iteration for Streamlines but not for ArraySequence. A Streamlines IS an ArraySequence after all. There's no way I can (or want to) add anything specific to trk in ArraySequence. In fact, ArraySequence should be in another crate because it's not actually related to trk :) I already use it for other things, like ArraySequence<usize>.

Knowing this, we simply need to go a little further into NiBabel. I already wrote about TractogramItem. I suggest we also add Tractogram, which will contain a Streamlines, Scalars, Properties and everything else required to do its jobs.*

What's interesting is that this solves one of our problems.

// If the user wants only the points
for streamline in tractogram.streamlines {}

// If he also wants s/p:
for (streamline, scalars, properties) in tractogram {}

* Yes, "just like NiBabel". nibabel.streamlines was made by a knowledgeable programmer and, IMO, we should take some of their good ideas and leave what we dislike.

@fmorency
Copy link

I like it.

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

No branches or pull requests

2 participants