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

Feature: TC surge from bathtub model #97

Merged
merged 4 commits into from
Nov 30, 2020

Conversation

tovogt
Copy link
Collaborator

@tovogt tovogt commented Nov 26, 2020

Due to problems with the elevation package under Windows (issue #9), the old surge module has been removed from the main and develop branches and archived under archive/feature/tcsurge. A completely new TC surge module is in the pipeline in the feature/tc_surge_geoclaw branch. However, due to it's heavy computational needs, there is still some interest in the old simplified approach, most recently by @QinhanZhu and @arunranain who managed to remove the dependency on the elevation package from the old module.

I added some references from the literature, implemented unit tests and modified the tutorial notebook. As discussed in #9, users are now required to provide their own DEM data set as a raster file. Two freely available data sets are mentioned in the tutorial notebook. I think, the module is now ready to be merged back into the develop branch.

Naming: I called the old model a "bathtub" model to make clear that it's a very simplified approach. The term "bathtub model" is often used in the storm surge literature to refer to a model that ignores all hydrodynamics and instead basically subtracts a digital elevation model from an assumed surge height.

@chahank: Please feel free to assign more/other reviewers.

@chahank
Copy link
Member

chahank commented Nov 26, 2020

Were these changes discussed with @QinhanZhu and @arunranain ?

@chahank chahank requested review from Evelyn-M and aleeciu and removed request for chahank November 26, 2020 18:32
Copy link
Collaborator

@Evelyn-M Evelyn-M left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the work, I just have a few, mostly conceptual / readability comments.
Nothing to remark on the jupyter notebook, looks good!

climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
@tovogt
Copy link
Collaborator Author

tovogt commented Nov 27, 2020

Thanks for the valuable feedback @Evelyn-M! I will comment directly on each of your points.

Were these changes discussed with @QinhanZhu and @arunranain ?

They gave me access to their modified version and communicated that they don't plan to continue working on a transformation into an update for the CLIMADA core.

@tovogt tovogt linked an issue Nov 27, 2020 that may be closed by this pull request
@chahank
Copy link
Member

chahank commented Nov 27, 2020

Thanks for the valuable feedback @Evelyn-M! I will comment directly on each of your points.

Were these changes discussed with @QinhanZhu and @arunranain ?

They gave me access to their modified version and communicated that they don't plan to continue working on a transformation into an update for the CLIMADA core.

Is the new implementation compatible with the way they used the TC_surge module for the Vietnam study? Because they are also modifying the TC_surge module at the moment to make it possible to use any DEM model. The crux is the clipping part. I just want to be sure that not two competing version of the same module are developed here.

@tovogt
Copy link
Collaborator Author

tovogt commented Nov 27, 2020

Is the new implementation compatible with the way they used the TC_surge module for the Vietnam study? Because they are also modifying the TC_surge module at the moment to make it possible to use any DEM model. The crux is the clipping part. I just want to be sure that not two competing version of the same module are developed here.

As I said, they gave me access to their code and no, they are not modifying it at the moment - they actually stopped working on it. If I'm not mistaken, my PR is 100% compatible with what they did and it's supposed to reproduce the exact behavior of the legacy tc_surge module with a somewhat polished API and improved implementation. The API changes I introduced only affect parts of the API that are not used for that Vietnam study (as I said, I have access to their code - I checked this).

@chahank
Copy link
Member

chahank commented Nov 27, 2020

OK great. Just wanted to be sure. But could you please double check with @QinhanZhu as I talked to him recently and (probably I misunderstood) he seemed to imply to wanting to change something about the clipping of DEM in TC_surge.

climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
climada/hazard/tc_surge_bathtub.py Outdated Show resolved Hide resolved
@tovogt
Copy link
Collaborator Author

tovogt commented Nov 30, 2020

Thanks @emanuel-schmid, I addressed all of your points in my latest commit.

@emanuel-schmid emanuel-schmid merged commit 40f7487 into develop Nov 30, 2020
@emanuel-schmid emanuel-schmid deleted the feature/tc_surge_bathtub branch November 30, 2020 10:04
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.

elevation package
4 participants