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.

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

t.rast.neighbors: support all r.neighbors features and allow to append to existing STRDS as well as filtering by region #3798

Merged
merged 14 commits into from
Jul 19, 2024
Merged

t.rast.neighbors: support all r.neighbors features and allow to append to existing STRDS as well as filtering by region #3798

merged 14 commits into from
Jul 19, 2024

Conversation

ninsbl
Copy link
Member

@ninsbl ninsbl commented Jun 11, 2024

This PR:

  • propagates features (options and flags) from r.neighbors to t.rast.neighbors that were available in the underlying module, but not it's TGIS version
  • adds filtering by computational region
  • allows to append results to an existing STRDS
  • reactivates and extends the tests

We may remove the SHELL version of the tests...

@ninsbl ninsbl added enhancement New feature or request temporal Related to temporal data processing Python Related code is in Python labels Jun 11, 2024
@ninsbl ninsbl added this to the 8.5.0 milestone Jun 11, 2024
@github-actions github-actions bot added module tests Related to Test Suite labels Jun 11, 2024
@ninsbl ninsbl requested review from veroandreo and metzm June 12, 2024 07:31
@ninsbl
Copy link
Member Author

ninsbl commented Jun 12, 2024

See also #3427

@ninsbl ninsbl mentioned this pull request Jun 12, 2024
Open
17 tasks
@ninsbl ninsbl modified the milestones: 8.5.0, 8.4.0 Jun 12, 2024
veroandreo
veroandreo previously approved these changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@veroandreo veroandreo left a comment

Choose a reason for hiding this comment

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

After fixing n coordinate, tests run successfully. Is there anything else I should test particularly? If yes, an example in the manual page, would be great :)

temporal/t.rast.neighbors/t.rast.neighbors.py Outdated Show resolved Hide resolved
temporal/t.rast.neighbors/t.rast.neighbors.py Outdated Show resolved Hide resolved
temporal/t.rast.neighbors/t.rast.neighbors.py Outdated Show resolved Hide resolved
temporal/t.rast.neighbors/testsuite/test_neighbors.py Outdated Show resolved Hide resolved
@ninsbl
Copy link
Member Author

ninsbl commented Jun 15, 2024

The manual is now also updated with the changes in this PR.

@github-actions github-actions bot added HTML Related code is in HTML docs labels Jun 15, 2024
@ninsbl ninsbl requested a review from veroandreo June 16, 2024 19:38
veroandreo
veroandreo reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@veroandreo veroandreo left a comment

Choose a reason for hiding this comment

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

I tested again, within the world_latlong project, all tests are OK. However, see my comment below regarding the warning that is printed when running them. Is this correct?

temporal/t.rast.neighbors/t.rast.neighbors.py Outdated Show resolved Hide resolved
@ninsbl
Copy link
Member Author

ninsbl commented Jul 14, 2024

The fix in 4e6f5bf should probably be backported to 8.4...

@neteler
Copy link
Member

neteler commented Jul 14, 2024

The fix in 4e6f5bf should probably be backported to 8.4...

Should this still go into 8.4.0 or 8.4.1?

@ninsbl
Copy link
Member Author

ninsbl commented Jul 16, 2024

The fix in 4e6f5bf should probably be backported to 8.4...

Should this still go into 8.4.0 or 8.4.1?

I think 8.4.1 can be sufficient as it does not seem to cause issues other places in main. Not sure about addons though...
I can do a quick backport 8.4.0 if that is preferable...

@ninsbl ninsbl requested a review from veroandreo July 17, 2024 21:12
@ninsbl ninsbl mentioned this pull request Jul 17, 2024
Merged
@echoix
Copy link
Member

echoix commented Jul 19, 2024

@ninsbl I think it's ready, but just before, can you try (locally) if with ruff 0.5.3 there's no changes (PLR17xx something changed and covers a new case than before). I don't think it's worth a full new run of CI, but just check again, it takes less than a second to run ;)

@ninsbl
Copy link
Member Author

ninsbl commented Jul 19, 2024

No changes with ruff 0.5.3 detected...

@ninsbl ninsbl merged commit 7840f76 into OSGeo:main Jul 19, 2024
26 checks passed
@ninsbl ninsbl deleted the t_rast_neighbors_update branch July 19, 2024 06:33
Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
@ninsbl @veroandreo @Mahesh1998
t.rast.neighbors: support all r.neighbors features and allow to appen…
66cc4b7 
…d to existing STRDS as well as filtering by region (OSGeo#3798)

* propagate r.neighbors options

* reactivate and extend tests

* allow to extend existing STRDS

* allow spatial selection by computational region


---------

Co-authored-by: Veronica Andreo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veroandreo veroandreo veroandreo approved these changes

@metzm metzm Awaiting requested review from metzm

Assignees
No one assigned
Labels
docs enhancement New feature or request HTML Related code is in HTML libraries module Python Related code is in Python temporal Related to temporal data processing tests Related to Test Suite
Projects
None yet
Milestone
8.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ninsbl @neteler @echoix @veroandreo