Skip to content

Add option to merge ahn tiles#431

Merged
martinvonk merged 11 commits intodevfrom
solve_issue_429
May 1, 2025
Merged

Add option to merge ahn tiles#431
martinvonk merged 11 commits intodevfrom
solve_issue_429

Conversation

@rubencalje
Copy link
Collaborator

No description provided.

@martinvonk martinvonk linked an issue Mar 19, 2025 that may be closed by this pull request
@rubencalje
Copy link
Collaborator Author

So a lot of changes, @martinvonk . Can you explain what the purpose of these changes is? And I think we should use type hints everywhere in NLmod, or no type hints at all (which I like more).

@martinvonk
Copy link
Collaborator

I tried to add some code to use the old _download_and_combine_tiles function to merge the separate tiles. This function used a memoryfile thingy, and the merge function instead of merge_arrays which might be a solution to #429.

And yes, I added some type hints 👼. Originally because I was confused about the expected input for some functions. Later more, because I thought it is nice to be explicit in the function call which specific version (strings) need to be provided for an AHN request. Btw, there are already some type hints in NLmod. I'm always in favor of more documentation instead of less.

@rubencalje
Copy link
Collaborator Author

I tried to add some code to use the old _download_and_combine_tiles function to merge the separate tiles. This function used a memoryfile thingy, and the merge function instead of merge_arrays which might be a solution to #429.

And yes, I added some type hints 👼. Originally because I was confused about the expected input for some functions. Later more, because I thought it is nice to be explicit in the function call which specific version (strings) need to be provided for an AHN request. Btw, there are already some type hints in NLmod. I'm always in favor of more documentation instead of less.

So does it work? Can you add a test?

This PR was meant as a quick fix so you could use the code in #429 (comment), which solved your problem. I feel like this PR is hijacked by some of your preferences.

@martinvonk
Copy link
Collaborator

So does it work? Can you add a test?

It does not work yet :) But I'll have a look to see if I can fix it.

Otherwise, I'll look if I can catch the logging info shown in #429, and throw an error message to use the return_tiles argument.

@martinvonk martinvonk marked this pull request as ready for review May 1, 2025 09:39
@martinvonk
Copy link
Collaborator

martinvonk commented May 1, 2025

I gave up on trying to catch the logger.info warning of rioaxxary/rasterio and raising a warning to not merge the ahn tiles.

Only thing I changed compared to @rubencalje is that I renamed return_tiles to 'merge_tiles`. That way it is more clear that we do an operation on the data. And I added some type hints :).

My suggestion would be to close #429 as won't fix because it is basically a rioxarray issue. If the extent is too large, and the function/logging.info keeps going on forever; users themselves have to figure out that there is an issue. They'd have to find #429 and try @rubencalje's solution.

@martinvonk martinvonk changed the title Add return_tiles argument Add option to merge ahn tiles May 1, 2025
@martinvonk martinvonk merged commit e688283 into dev May 1, 2025
4 of 5 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in NHFLO May 1, 2025
@rubencalje rubencalje deleted the solve_issue_429 branch December 17, 2025 11:22
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.

Merging AHN tiles takes forever

2 participants