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

Adding your own topobathy data does not work #162

Open
2 tasks done
kathrynroscoe opened this issue Oct 10, 2024 · 5 comments · Fixed by #164
Open
2 tasks done

Adding your own topobathy data does not work #162

kathrynroscoe opened this issue Oct 10, 2024 · 5 comments · Fixed by #164
Assignees
@roeldegoede
Labels
bug Something isn't working

Comments

@kathrynroscoe
Copy link
Collaborator

Preliminary Checklist

  • I have searched the existing issues list for similar reports and found none. If you did find a similar issue, please add a comment to the existing issue instead of opening a new one.
  • I have verified that the issue exists in the latest version of DelftDashboard and its dependencies. Please update to the latest version and check if the issue still exists.

Severity Level

High

Steps to reproduce

Add your own .tif DEM and you get the error below

image

Current behaviour

Adding your own .tif DEM throws an error

Desired behaviour

Adding your own .tif DEM would work

Additional context

No response
@kathrynroscoe kathrynroscoe added the bug Something isn't working label Oct 10, 2024
@roeldegoede
Copy link
Collaborator

As discussed with @LuukBlom later that day, this is the way it is intended to be. The overviews in the dataset speed up both building the model and visualizing the dataset through the top menu in the "topography" tab. We could let go this "requirement, but then I'm pretty sure we will be runnning into memory issues whe visualizing the dataset.

I have been thinking to automate this in the background, i.e. autoamtically adding the overviews, but this really changes existing datasets (that also might be quite big). Hence, I would propose to properly document on how to add these overviews, since there is plenty of options to do so, e.g. in QGIS, Python notebook or just command line code.

One thing that is really broken, is the fact that "newly added datasets" only can be used once you restart the GUI ... This is because we have to re-initialize the model with the newly created data-catalog. As discussed with Luuk, we might just throw a warning saying something like "Adding your own datasets is beta functionality. Unfortunately, you have to restart the GUI to be able to use and visualize your own dataset ..."

@LuukBlom
Copy link
Collaborator

I added the automatic creation of the overviews to the gui including some warnings that this will increase the size and change the file + a warning that the user needs to restart to loading the datasets in the FloodAdapt ModelBuilder branch:
see this PR: src/delftdashboard/toolboxes/modelmaker_sfincs_hmt/bathymetry.py

Since this is the FA branch, I thought that'd be okay.
Let me know what you think!

@kathrynroscoe
Copy link
Collaborator Author

kathrynroscoe commented Oct 11, 2024 via email

@kathrynroscoe
Copy link
Collaborator Author

kathrynroscoe commented Oct 11, 2024 via email

@roeldegoede
Copy link
Collaborator

Okay cool, that's fine with me! One thing that you should change, that I didn't mention before, is that we would like to resample them using "averaging" method: process = subprocess.run(["rio", "overview", "--build", "auto", "--resampling", "average", fn[0]])

And I usually used rasterio functionality rather than the subprocess one, though they essentially should do the same ..

with rasterio.open(file, 'r+') as src:
    # check if overviews are present
    if src.overviews(1):
        print('Overviews already present in {}.'.format(file))
        continue

    # create new overviews, resampling with average method
    src.build_overviews([2, 4, 8, 16, 32, 64], Resampling.average)
    
    # update dataset tags
    src.update_tags(ns='rio_overview', resampling='average')

Anyway, this is not really important and we could maybe check performance of the latter, but this was just to stick a bit more to the already existing code (where we already have the dataset open with rasterio). Maybe also good to add a link to the manual of rasterio? https://rasterio.readthedocs.io/en/latest/topics/overviews.html Think this gives some more background of why we add these overviews?

@LuukBlom LuukBlom linked a pull request Oct 11, 2024 that will close this issue
Use rasterio directly instead of subprocess #164
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@roeldegoede roeldegoede

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use rasterio directly instead of subprocess Deltares-research/DelftDashboard
3 participants
@kathrynroscoe @roeldegoede @LuukBlom