Update init method for storm_europe#562
Conversation
zeliest
force-pushed
the
feature/init_hazard_storm_europe
branch
from
710278a to
236d533
Compare
…' into feature/init_hazard_storm_europe
climada/hazard/storm_europe.py
Outdated
| ssi_full_area: Optional[np.ndarray] = None | ||
| ): | ||
| """Calls the Hazard init dunder. Sets unit to 'm/s'.""" | ||
| Hazard.__init__(self, HAZ_TYPE) |
There was a problem hiding this comment.
parameter description is missing in pydoc string
There was a problem hiding this comment.
I don't have any experience with this kind of Hazard. From what I see in the code, ssi and ssi_wisc seem to be one dimensional arrays with one entry for each event. But the ssi_full_area attribute seems to be exclusively used for probabilistic events (see my other comment), and I don't know what this has to do with a "full area".
There was a problem hiding this comment.
In order to finalize the docstrings, we will probably need input from somebody who is familiar with this hazard type...
mmyrte
left a comment
mmyrte
left a comment
There was a problem hiding this comment.
I was first going "oh my, this is fancy, passing references to output arrays", but when I saw what I had done further down, it made sense. I'm happy with the changes, thanks!
|
@mmyrte Can you please have a look at the docstring of the |
climada/hazard/storm_europe.py
Outdated
| The Storm Severity Index (SSI) ???. Shape ``(?? x ??)``. Defaults to an empty | ||
| array. | ||
| """ | ||
| Hazard.__init__(self, haz_type=HAZ_TYPE, units=units, **kwargs) |
There was a problem hiding this comment.
Actually, why not super().__init__ here? There might be a good reason so I'm curious.
There was a problem hiding this comment.
No particular reason I guess. super() would be cleaner: you could change the super class by changing the one line of code that declares the class. But I don't expect StormEurope to inherit from anything else then Hazard ever.
@tovogt Sorry, was otherwise occupied. The unit tests indicate that the SSI is a 1d vector of equal length as the number of events. I'll check out the branch and ammend those lines, just a moment. |
climada/hazard/storm_europe.py
Outdated
| 1d vector, same length as number of storms. Defaults to an empty | ||
| array. | ||
| ssi_full_area : numpy.ndarray | ||
| FIXME need to recover rationale for this attribute; no substantive |
There was a problem hiding this comment.
@ThomasRoosli do you recall why we had this additional ssi_full_area thing in here? It's only ever used when computing a synthetic event set. I seem to remember that there was a good reason to not set the synthetic SSI to the same attribute name present in the reanalysis/WISC synthetic data. However, I don't remember, and we seem not to have documented it. I propose to simply drop this additional parameter. I would say this PR is as good an occasion as any.
There was a problem hiding this comment.
I would have to look closer into the code. From my memory: ssi_full_area is needed to conserve the index that is derived from the area and windspeed of the full geographic extend of a storm system. This is importent if a StormEurope object is cropped to a smaller region (e.g. a country).
There was a problem hiding this comment.
@ThomasRoosli: thanks for chipping in! In this case I'd be fine to keep the attribute and write just that into the description:
Used to conserve the the index that is derived from the area and windspeed
of the full geographic extend of a storm system. Needed for cropping the
`StormEurope` object into a smaller region`
* Instantiate StormEurope objects with full signature where possible. * Promote StormEurope._read_one_nc to classmethod.
|
There were quite a few instances where the new init was not used. I fixed that in 3d5f54e. The rest looks fine to me, apart from the open discussions on the specific |
climada/hazard/storm_europe.py
Outdated
| LOGGER.info( | ||
| "Omitting files %s", [ | ||
| file for file in file_names if file in files_omit]) |
There was a problem hiding this comment.
| LOGGER.info( | |
| "Omitting files %s", [ | |
| file for file in file_names if file in files_omit]) | |
| if any(fo in file_name for fo in files_omit): | |
| LOGGER.info("Omitting files %s", [ | |
| file for file in file_names if file in files_omit]) |
There was a problem hiding this comment.
Very important suggestion, thank you! But I did not want to add yet another list comprehension. I found a different solution using sets, see 6f0acc1
There was a problem hiding this comment.
Rather very unimportant, right? But yes, sets make more sense. 👍 (I was thinking along the same lines. Let's just hope the list was rather a bug than e feature, i.e., providing a path list with duplicates is not an intended valid use case. 🤞)
|
@peanutfun Good catch! 🙌 Also nice, to leave the fraction as |
* Fix unbound variables. * Add test case for 'files_omit' argument.
7 participants
Changes proposed in this PR:
This PR fixes issue #
PR Author Checklist
develop)PR Reviewer Checklist