Skip to content

support non-square shaped tiles#15

Open
JulesScholler wants to merge 3 commits into
masterfrom
rectangle_frame
Open

support non-square shaped tiles#15
JulesScholler wants to merge 3 commits into
masterfrom
rectangle_frame

Conversation

@JulesScholler
Copy link
Copy Markdown
Contributor

I added the support of non-squarred sensors. I ran the tests locally and they passed. I also try on squarred and non suarred data sets and it worked.

@JulesScholler JulesScholler self-assigned this Dec 1, 2023
Comment thread paprica/loader.py Outdated
neighbors_path=None,
frame_size=2048,
frame_size_h=apr.shape(1),
frame_size_v=apr.shape(2),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

given our weird convention of (z, x, y) corresponding to D, H, W, should these be swapped?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or are the frames arrays of shape (frame_size_h, frame_size_v)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So python is (z, y, x) and that corresponds to (z, v, h) so I think v is indexed by 2 and h by 1, am I wrong?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

frames are (z, frame_size_v, frame_size_h), I used v for vertical and h for horizontal to avoid the ambiguity of (x, y, z)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So python is (z, y, x) and that corresponds to (z, v, h) so I think v is indexed by 2 and h by 1, am I wrong?

then they should be swapped -- apr.shape() should correspond to the stack shape in python. It's just that internally libapr swaps the name of x and y, it doesn't change the data. Super annoying, I know

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok I did not remember that pyapr internally remapped the axis, thanks for spotting it!

@joeljonsson
Copy link
Copy Markdown
Contributor

from the CI tests it seems like there may be an issue with python 3.7 and some dependency?

@JulesScholler
Copy link
Copy Markdown
Contributor Author

from the CI tests it seems like there may be an issue with python 3.7 and some dependency?

Seems like there is an issue with simpleITK, to be checked but I bet it's again allensdk that is poorly maintained.

@joeljonsson
Copy link
Copy Markdown
Contributor

joeljonsson commented Dec 1, 2023

from the CI tests it seems like there may be an issue with python 3.7 and some dependency?

Seems like there is an issue with simpleITK, to be checked but I bet it's again allensdk that is poorly maintained.

Well, python 3.7 is end of life, so many packages are dropping it. We may want to do the same (and include at least 3.10, but preferably 11 and 12 as well)

@JulesScholler
Copy link
Copy Markdown
Contributor Author

For now, I dropped python 3.7, ultimately I think it would be good to drop 3.9 and use the "switch case" feature of python 3.10 and support 3.11 and 3.12. I can't really put time on it right, but might in the future...

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