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

Reorganize unittests for tc_surge_geoclaw #148

Open
emanuel-schmid opened this issue Nov 21, 2024 · 15 comments
Open

Reorganize unittests for tc_surge_geoclaw #148

emanuel-schmid opened this issue Nov 21, 2024 · 15 comments

Comments

@emanuel-schmid
Copy link
Contributor

With #109 came a bunch of new unittests. One of them, climada_petals.hazard.tc_surge_geoclaw.test.test_tc_surge_geoclaw.TestHazardInit.test_init, takes 46 seconds on Jenkins (and, depending on the settings, up to 130 seconds on the Euler cluster).
According to our guide lines this is too much for any test let alone a unit test.

I suggest to

  1. immediate measure: move it to the integration test
  2. in the long run: rewrite the test so that it is done within reasonable time (<10sec)
@chahank
Copy link
Member

chahank commented Nov 21, 2024

Ok, I traced this back, and the time is mostly lost in the concatenating of hazard objects, and more precisely in the concatenation of the centroids. 31 of the 40 seconds are used by Centroids.union in Hazard.concat called in TCSurgeGeoClaw.from_tc_tracks .

@emanuel-schmid
Copy link
Contributor Author

🙌 awesome! Do you also know how this can be sped up?

@chahank
Copy link
Member

chahank commented Nov 22, 2024

I will definitely try to find a solution :D

@NicolasColombi
Copy link
Collaborator

On my laptop it takes 18s 🙄

Other issue: there is an integration test failing on my machine, cell 3 of the tutorial climada_hazard_TCSurgeGeoClaw.ipynb fails as well, with the same RunTimeError of the test. Does the following test pass on your computer ?
climada_petals.test.test_tc_surge_geoclaw.TestGeoclawRun.test_surge_from_track

The error is the same for both the test and the tutorial:

The warning preceding the error:
/Users/ncolombi/miniforge3/envs/climada_dev/lib/python3.9/site-packages/shapely/constructive.py:181: RuntimeWarning: invalid value encountered in buffer return lib.buffer(
The error:
RuntimeError: GeoClaw run failed

@chahank
Copy link
Member

chahank commented Nov 22, 2024

The test also fails for me, but the error message is:

File ~/Documents/Climada/climada_petals/climada_petals/hazard/tc_surge_geoclaw/tc_surge_geoclaw.py:595, in _geoclaw_surge_from_track(track, centroids, topo_path, max_dist_eye_deg, geoclaw_kwargs, pool)
    593 else:
    594     for runner in runners:
--> 595         runner.run()
    597 surge_h = []
    598 for event, runner in zip(events, runners):

File ~/Documents/Climada/climada_petals/climada_petals/hazard/tc_surge_geoclaw/geoclaw_runner.py:215, in GeoClawRunner.run(self)
    213     self.stdout = self.work_dir.joinpath("stdout.log").read_text()
    214 else:
--> 215     self._run_subprocess()
    216 LOGGER.info("Reading GeoClaw output ...")
    217 try:

File ~/Documents/Climada/climada_petals/climada_petals/hazard/tc_surge_geoclaw/geoclaw_runner.py:270, in GeoClawRunner._run_subprocess(self)
    268 if proc.returncode != 0 or stopped:
    269     self.print_stdout()
--> 270     raise RuntimeError("GeoClaw run failed (see output above).")

RuntimeError: GeoClaw run failed (see output above).

@chahank
Copy link
Member

chahank commented Nov 22, 2024

Ok, so the unit test can be made very quick by making the centroids smaller. Fix propose in PR: #149

@emanuel-schmid
Copy link
Contributor Author

On Euler the said test fails in yet another way:

[...]
2024-11-22 16:37:08,323 - climada_petals.hazard.tc_surge_geoclaw.geoclaw_runner - INFO - Output of 'make .output' in GeoClaw work directory:
/cluster/work/sis/cdss/schmide/climada/data/geoclaw/src/clawpack/clawutil/src/Makefile.common:111: *** recipe commences before first target.  Stop.

E
======================================================================
ERROR: test_surge_from_track (climada_petals.test.test_tc_surge_geoclaw.TestGeoclawRun.test_surge_from_track)
Test _geoclaw_surge_from_track function (~30 seconds on a notebook)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/cluster/project/sis/cdss/schmide/climada_petals/climada_petals/test/test_tc_surge_geoclaw.py", line 98, in test_surge_from_track
    intensity, gauge_data = _geoclaw_surge_from_track(
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/cluster/project/sis/cdss/schmide/climada_petals/climada_petals/hazard/tc_surge_geoclaw/tc_surge_geoclaw.py", line 595, in _geoclaw_surge_from_track
    runner.run()
  File "/cluster/project/sis/cdss/schmide/climada_petals/climada_petals/hazard/tc_surge_geoclaw/geoclaw_runner.py", line 215, in run
    self._run_subprocess()
  File "/cluster/project/sis/cdss/schmide/climada_petals/climada_petals/hazard/tc_surge_geoclaw/geoclaw_runner.py", line 270, in _run_subprocess
    raise RuntimeError("GeoClaw run failed (see output above).")
RuntimeError: GeoClaw run failed (see output above).

----------------------------------------------------------------------
Ran 1 test in 40.183s

@emanuel-schmid
Copy link
Contributor Author

emanuel-schmid commented Nov 22, 2024

This Geoclaw thingy seems to be somewhat experimental...
I wonder whether we should revert that merge 🧐

@chahank
Copy link
Member

chahank commented Nov 22, 2024

Just FYI: This is the same error as the one I get.

I looked into the Makefile.common:111 file line 11, and there is the comment: # Note that there should be a space after this flag. However, I cannot see the space. Maybe this is the problem? I however did not yet understand where this space would have to be added. I also do not know where this would have to be solved.

@saeedsvz
Copy link

saeedsvz commented Dec 9, 2024

Hi Emanuel, it seems like the module (tc_surge_geoclaw) has been deleted from the package. Am I right? I cannot fin it under the climate_petals directory.

@chahank
Copy link
Member

chahank commented Dec 10, 2024

The package has not been deleted. I am not sure what you are missing.

@saeedsvz
Copy link

Hi @chahank, I am looking for this "climada_petals.hazard.tc_surge_geoclaw" and I cannot find it under climate_petals.

@chahank
Copy link
Member

chahank commented Dec 10, 2024

@saeedsvz
Copy link

I got it, thank you so much for your help.

@peanutfun
Copy link
Member

just FYI: The GeoClaw runner will run the GeoClaw executable in a subprocess, and will first try to compile it from Fortran if it does not exist (hence the Makefile execution). Compiling is a highly system-dependent operation, so it's not surprising that this fails in different ways on various systems.

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

No branches or pull requests

5 participants