CLIMADA-project / climada_petals

See https://github.com/CLIMADA-project/climada_python first
GNU General Public License v3.0
22 stars 13 forks source link

tc_surge_geoclaw: surge flooding from dynamic flow solver GeoClaw #109

Open tovogt opened 8 months ago

tovogt commented 8 months ago

Changes proposed in this PR:

This new hazard class models the storm surge impact from tropical cyclone tracks. Internally, the fluid dynamics solver GeoClaw is used to simulate the storm surge dynamics. Please refer to the tutorial notebook for more information.

It's important to note that this functionality is not for the average user of CLIMADA: It requires a Fortran compiler, a non-trivial choice of input data as well as large computing power. The example of a single (!) TC track in the tutorial notebook takes a few minutes to run on a laptop computer. But this is only possible because the resolution is set to 10 km (300 arc-seconds). In practice, a resolution of 1 km (30 arc-seconds) is reasonable, and will require 3 hours even on a HPC cluster with 16 cores. All this is mentioned in the code, docstrings and in the tutorial notebook.

Because of that, the unit tests for this subpackage do not include non-trivial model runs. However, there is a non-trivial integration test that runs on a modern laptop computer in approximately one minute.

@emanuel-schmid Note that the code requires new datasets in the CLIMADA API: data.zip

test_altimetry_tubuai_2010_900as.nc : name='test_altimetry_tubuaix10', status='test_dataset'
test_bathymetry_tubuaix10_300as.tif : name='test_bathymetry_tubuai, status='test_dataset'
tutorial_altimetry_gulf_2005_900as.nc : name='tutorial_altimetry_gulf', status='package-data'
tutorial_bathymetry_gulf_300as.tif : name='tutorial_bathymetry_gulf', status='package-data'

PR Author Checklist

PR Reviewer Checklist

tovogt commented 6 months ago

@emanuel-schmid Would it be possible that you add the data files mentioned in my OP to the data API so that we can see whether the tests are fine?

@peanutfun Is there any chance that this will be reviewed by you or somebody else in the next two weeks? The background is that I will leave PIK afterwards and won't continue work on this. Others in my group will pick up my GeoClaw stuff, and I'm wondering whether I need to instruct them to finalize this PR.

I now squashed all the commits to remove the clutter from this PR.

emanuel-schmid commented 5 months ago

@emanuel-schmid Would it be possible that you add the data files mentioned in my OP to the data API so that we can see whether the tests are fine?

Sure. do they belong to any particular data-type, or are "base" for the package-data and "test" for the test_data suiting?

tovogt commented 5 months ago

Sure. do they belong to any particular data-type, or are "base" for the package-data and "test" for the test_data suiting?

"base" for all of them would be fine, but having "base" for the package-data and "test" for the test_data would also make sense. I don't think that we can be much more specific than that.

emanuel-schmid commented 5 months ago

the data is available now. on jenkins, there are still problems with the clawpack module availability and the fortran compiler: https://ied-wcr-jenkins.ethz.ch/job/petals_branches/view/change-requests/job/PR-109/9/testReport/

I'll check whether conda can help...

tovogt commented 5 months ago

Oh yes, there is a new requirement meson-python in requirements/env_climada.yml. When will Jenkins pick that up?

emanuel-schmid commented 5 months ago

After merging and a day. But one can install additional branch specific packages through scripts/jenkins/branches/Jenkinsfile, as in d7d0d611fa7a223f4fae3cf90d62df3f9f4bbbb5

emanuel-schmid commented 5 months ago

It runs through now. Only problem: the unittest coverage is lower than avarage and our high quality standards forbid that! Hence it fails on Jenkins 😜 (Guess I still need to do some fine tuning in the CI coverage configuration.)

emanuel-schmid commented 5 months ago

Another problem: geoclaw doesn't support Windows see https://github.com/clawpack/clawpack/issues/80, although ancient, it's still around. If I had to guess - may be about symbolic links?

tovogt commented 5 months ago

Another problem: geoclaw doesn't support Windows see clawpack/clawpack#80, although ancient, it's still around. If I had to guess - may be about symbolic links?

Hm, I think that fixing Windows support of GeoClaw/Clawpack is out of scope for this PR. Is there a rule that we cannot have code in CLIMADA that will not run on Windows? In practice, running this tc_surge_geoclaw module under Windows does not make any sense anyways. For practicable applications, you will always want to run this code on an HPC cluster.

emanuel-schmid commented 5 months ago

Agreed, fixing Windows is out of scope.

AfaIk we don't have such a rule, but so far we have been able to avoid OS distinctions. (And honestly I feel somewhat uneasy about opening this can 😱,) But then: this is petals so I guess we have some liberty. 🤠

In any case we cannot assume the user to be not on Windows, so we would probably need

would that be in scope?

tovogt commented 5 months ago

The unit tests on Jenkins are all successful now. But the tests on GitHub CI still fail because the new requirement (meson-python) is missing.

I increased the coverage by implementing some more unit tests. This part of the checks is fine now. Note that some parts of the code can only be covered by integration test because they have higher computational demands.

As for the linting: There are 6 "high" priority linting issues ("no-member") where pylint fails to recognize the correct type of an object or fails to recognize generated members of pandas DatetimeIndex instances. I don't think there is much we can do about this other than ignore. Unfortunately, I wasn't able to find the right thing to put in pylintrc to ignore this kind of issue.

In any case we cannot assume the user to be not on Windows, so we would probably need

* docs tell them ("works only on Linux and Mac")
* code fails gracefully (with a clear statement: "this cannot work on Windows")
* unit tests skip the geoclaw test on Windows

Great idea! I addressed all three points:

emanuel-schmid commented 5 months ago

Looks all good to me 😃 . We only need to either install a fortran compiler in GitHub actions or skip the tests there too.

tovogt commented 5 months ago

I tend to prefer installing a Fortran compiler in GitHub actions if that's not overly complicated...?

peanutfun commented 5 months ago

@tovogt The GitHub Actions automatically incorporate changes to env_climada.yml. As far as I can see from the tests, meson-python is installed correctly during the pipeline. If you additionally need a fortran compiler for the module, I suggest to add it to the dependencies, too. If that is not desired for some reason, you can update the environment manually in the pipeline with another call to micromamba install, see https://github.com/CLIMADA-project/climada_petals/blob/123627c602a5493fc49455523fe4d65c89911335/.github/workflows/testing.yml#L75

Note that tests are currently failing during the collection process, because importing the test_tc_rainfield.py module loads test data that is incompatible to climada v5. I guess this will be fixed with #122?

tovogt commented 5 months ago

The compiler packages in conda used to have OS-specific names. But luckily, this has changed in the mean time, and I can now add gfortran as a conda requirement. I think that should be the easiest solution.

For the other part, the goal would be that #122 fixes the collection process.