refactor bounding box

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊

[giovp]

I would need this refactor in order to move on with the data loader performance PR. @LucaMarconato what this PR basically does is unifying the building request code for DataArray|DataTree with the already existing _get_bounding_box_corners_in_intrinsic_coordinates for DaskDataFrame|GeoPandasDataFrame. I think this is correct, as in the tests for spatial_query pass, but I am not sure that the logic is correct.

I had to disable the tests for the dataloaders but they will be worked out in a separate PR. This PR is ready to review, but I would not merge it for now, and wait for finishing the dataloader improvement work.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (c3ad40c) to head (7627f9f).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata/_core/query/spatial_query.py	89.28%	3 Missing ⚠️
src/spatialdata/dataloader/datasets.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #617 +/- ##
==========================================
- Coverage 91.93% 90.04% -1.90% 
==========================================
Files 44 44 Lines 6659 6670 +11 ==========================================
- Hits 6122 6006 -116 - Misses 537 664 +127 

Files with missing lines	Coverage Δ
src/spatialdata/dataloader/datasets.py	33.65% <50.00%> (-57.03%)	⬇️
src/spatialdata/_core/query/spatial_query.py	94.64% <89.28%> (-0.53%)	⬇️

... and 7 files with indirect coverage changes

[LucaMarconato]

(deleted)

[LucaMarconato]

Great PR! It simplifies the code and will make the torch dataset class more performant!

I added a few comments of changes I would make. They are all minor except for one that may require more care (the one where axes and output_axes_without_c are swapped). I can also have a deeper look at that case if you want.

[giovp]

Hi @LucaMarconato , thank you for the review, I've addressed all comments, except this one

one that may require more care (the one where axes and output_axes_without_c are swapped)

If I don't swap the axes, there is one test that fails (see review comment). I think it might be solvable by changing the input/output axes ofspatial_transform_bb_axes but I am not sure the best solution there. The fact that it's also only that test for spatialdata, and not the e.g. test_query_points also makes it a bit suspicious. Would really appreciate your help!

[giovp]

one more question, is return_request_only a good argument name? and should it be in all cases or only raster? I would argue only raster because I don't see how much value it has for the shapes/points element case. In fact, the only use case so far is for the dataloader.

[LucaMarconato]

Thanks for addressing the comments. I think the name is good and I agree in having it only for the raster case. If it will be needed later on we can always add it for the other elements.

I will examine more the last open issue, after we resolve the conversation on that we are good to merge.

[LucaMarconato]

@giovp I checked the code and the issue was a bug in the function _adjust_bounding_box_to_real_axes(), which was not adjust the bounding box to the real axes in the case in which the bounding box axes and the real axes were a permutation of each other. I have addressed that and now the swap between the two variables for non-raster types is removed.

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

[giovp]

Before the swap was fixing the bug because of a coincidence. I will add a targeted test for this case, and after that we can merge.

🙏 🙏

[LucaMarconato]

I have finished adding tests, good to merge (thanks for addressing the other review comments)! 😊