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

Use consistently count and indexes for band listing and counting (removing nbands and bands) #346

Merged
merged 13 commits into from
Feb 9, 2023

Conversation

rhugonnet
Copy link
Contributor

To be fully consistent with rasterio.

Resolves #280

Copy link
Member

@adehecq adehecq left a comment

Choose a reason for hiding this comment

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

Hopefully it will be much clearer now!
But what I don't get is that basically here you just removed nbands into count and bands into indexes. But I thought we had a different use of count and nbands, depending on what was loaded and what was on disk, no?
In the issue, I suggested using a count_loaded and indexes_loaded but it seems like it was not needed, why?

@rhugonnet
Copy link
Contributor Author

My bad, I forgot that point! I added them back and added tests for them.

The names count_loaded and indexes_loaded sounded good. But then count and index are "on-disk", and can take a value of None, which is not so practical when manipulating in-memory-only Raster. So instead I went for count_on_disk and indexes_on_disk, while count and indexes are in-memory. Clarified everything in the descriptions.

@rhugonnet rhugonnet merged commit 1b97323 into GlacioHack:main Feb 9, 2023
@rhugonnet rhugonnet deleted the homogenize_count_indexes branch February 9, 2023 23:33
@adehecq
Copy link
Member

adehecq commented Feb 16, 2023

My bad, I forgot that point! I added them back and added tests for them.

The names count_loaded and indexes_loaded sounded good. But then count and index are "on-disk", and can take a value of None, which is not so practical when manipulating in-memory-only Raster. So instead I went for count_on_disk and indexes_on_disk, while count and indexes are in-memory. Clarified everything in the descriptions.

Sounds good. Everything seems perfect now, thanks !

@adehecq adehecq mentioned this pull request Jan 22, 2024
5 tasks
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.

Fix the mess in attributes nbands/count/bands/indexes
2 participants