Skip to content

Improve error messages and make Raster indexing/assigment NumPy compatible #454

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 30, 2024
Merged

Improve error messages and make Raster indexing/assigment NumPy compatible #454

merged 8 commits into from
Jan 30, 2024

Conversation

rhugonnet
Copy link
Member

@rhugonnet rhugonnet commented Jan 26, 2024 •
edited
Loading

This PR:

  • Adds a consistent error message across operations requiring two raster or one raster and one array that are aligned (arithmetic, NumPy array interface, indexing, index assignment),
  • Removes the cropping behaviour of [] and makes it fully consistent with NumPy (allows any index supported by NumPy: integer, slices "1:5", ellipsis "...", or new axes.
  • Improves significantly the tests for all errors raised and new behaviour.

Mysterious behaviour of NumPy makes the tests fail only on 4 specific array functions, everything else passing... 🤔

Resolves #433
Resolves #431

Opened #457

@rhugonnet rhugonnet changed the title Improve error messages by proposing solutions Improve error messages and make indexing/assigment NumPy compatible Jan 28, 2024
@rhugonnet rhugonnet changed the title Improve error messages and make indexing/assigment NumPy compatible Improve error messages and make Raster indexing/assigment NumPy compatible Jan 28, 2024
@rhugonnet rhugonnet requested a review from adehecq January 28, 2024 00:37
@rhugonnet rhugonnet mentioned this pull request Jan 29, 2024
Open
adehecq
adehecq reviewed Jan 30, 2024
View reviewed changes
adehecq
adehecq reviewed Jan 30, 2024
View reviewed changes
adehecq
adehecq reviewed Jan 30, 2024
View reviewed changes
adehecq
adehecq reviewed Jan 30, 2024
View reviewed changes
adehecq
adehecq approved these changes Jan 30, 2024
View reviewed changes
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.

Nice that all these errors are consistently addressed now !
I only have a few minor comments.

@adehecq
Copy link
Member

adehecq commented Jan 30, 2024

Also according to coveralls, the test coverage has reduced a bit (2 lines) for vector.py with this PR.

@rhugonnet
Copy link
Member Author

I think it's because the lines don't exist anymore...
Since we added it a couple years ago, Coveralls doesn't seem to have picked up so well. Maybe it's time to switch to something that integrates more nicely with GitHub and saves us some time while reviewing!

@rhugonnet
Copy link
Member Author

Actually it looks like Coveralls is moving towards a GitHub integration too, maybe we can wait it out: https://coveralls.io/new-github-integration-beta

@rhugonnet rhugonnet merged commit fe8795c into GlacioHack:main Jan 30, 2024
@rhugonnet rhugonnet deleted the improve_error_messages branch January 30, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adehecq adehecq adehecq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Raise more intuitive errors for common operations Add indexing with slices to Raster.__getitem__
2 participants
@rhugonnet @adehecq