Remove intensity_thres attribute from Hazard class #1065
Conversation
|
@chahank: As we discussed, I now removed the intensity_thres attribute from the Hazard class. I kept it in the StormEurope and TropCyclone classes as it seems useful there. However, if you think it makes more sense to set the default to 0 instead of 17.5 for cyclones and 14.6 for winter storms, we can also do that. |
|
Great! Does setting the value to 0 change some results and/or impact performance? |
I did not check but I assume yes: Having values between e.g. 0 and 17.5 means there are many more nonzero entries in the sparse intensity matrix, and thus also in saved hazard files, right? I guess also the results would be marginally changed, depending on the impact function. |
|
ok, then we should probably leave it as is in the TC and winterstorm models. Maybe we can add a note in the docstring somewhere to make sure people know about it? |
| return_periods=(25, 50, 100, 250), | ||
| method="interpolate", | ||
| min_intensity=None, | ||
| min_intensity=0, |
There was a problem hiding this comment.
Does this mean that all sparse data is projected onto a dense matrix? This might lead to problems no?
There was a problem hiding this comment.
The calculation of local exceedance intensity is centroid wise, so yes, the sparse matrix is converted to a dense matrix column by column. The min_intensity parameter being zero or positive does not change this.
It might be that a positive min_intensity parameter here would result in larger geodataframes (ie. centroids with only very small intensities would still lead to a computation and nonzero values, instead of faster handling if all intensities are considered zero). However, as we do not have a minimum intensity for a general hazard object, I think this should be set to zero per default, and can be adapted by the user if wanted.
Does this answer your question?
Sounds good. We could add some info to the docstring of the two classes (StormEurope and TropCyclone)? Or do you mean the (class) methods that make use of these attributes? These should already have some info in the docstrings. |
This sounds good.
|
|
@chahank should we merge this? |
climada/hazard/storm_europe.py
Outdated
| intensity_thres = DEF_INTENSITY_THRES | ||
| """Intensity threshold for storage in m/s.""" |
There was a problem hiding this comment.
Sorry for the late extra comment, but is the class attribute then still needed? It looks like the intensity_thresh is now a parameter of all relevant methods no?
There was a problem hiding this comment.
Yes, intensity_thresh is an optional parameter of all relevant methods. This class attribute is used as the default values for these methods. If you change the attribute for a specific instance, the default value of all corresponding methods changes, and you do not have to change it manually each time.
Does this make sense? If there is a smarter implementation, happy to change this.
There was a problem hiding this comment.
It makes sense, although I am not sure the current implementation is doing what you want.
The code now has an class attribute, but actually never uses it, since the value of this attribute is always passed as parameter to all relevant methods. Thus, the class attribute seems redundant to me. What do you think?
There was a problem hiding this comment.
Yes, almost agree. There is one place in StormEurope and one in TropCyclone where the correpsonding class attributes are used (called on class instances though):
In StormEurope: in the function calc_ssi
In TropCyclone: in the function set_from_tracks which has a deprecation warning.
It looks to me as if we can remove these uses and then also remove the class attributes. I have to think about the calc_ssi though. If you confirm that it is better to remove them, I can try to implement that :)
There was a problem hiding this comment.
How about we add the parameter to the methods calc_ssi, remove the method set_from_tracks, adjust the method from_tracks (remove the haz.intensity_thres = intensity_thres), and remove the class parameter?
There was a problem hiding this comment.
I removed the class attribute from StormEurope and TropCyclone, and the deprecated TropCyclone.set_from_tracks. In the StormEurope.calc_ssi method, there was already an optional parameter threshold for the calculation of SSI. Before, the method checked if this parameter was larger than the class attribute intensity_thres and, if not, gave an error "threshold cannot be below threshold upon read_footprint". I don't understand why it should be a problem though if the calculation threshold is below the read-in threshold...
Let me know what you think, for me this looks good.
… TropCyclone.set_from_tracks
|
I have tried to check if there there is any problem with writing or reading "old" hazard files after removing the
This is code to see this in action, use e.g., main or develop branches. The first print command resulted in 18., the second in 14.7 (the default class attribute). from climada.hazard import StormEurope
from climada.util.constants import WS_DEMO_NC
storm_instance = StormEurope.from_footprints(WS_DEMO_NC)
storm_instance.intensity_thres = 18. # change intensity threshold
print(storm_instance.intensity_thres)
storm_instance.write_hdf5('storm_with_attribute.h5')
storm_instance = StormEurope.from_hdf5('storm_with_attribute.h5')
print(storm_instance.intensity_thres)Of course, in the new version, there is simply no @chahank Would this be enough check and can we merge? |
|
I think we just need to adjust the docstring to remove the class attribute mention. Also, 3 tests are failing, one for the supply chain module and one for the unsequa module. Not sure if this is related atm. |
The supply chain one looks unrelated; there is no hazard involved as far as I can see. For unsequa it could be due to the reading of the tropical cyclone datasets |
Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>
|
Thanks for checking the docstrings, this is now up to date. For the test, I don't know, for me locally it runs through. |
Ok, let's wait for the last round of tests. Do also the integration tests work for you locally? |
|
To me the failing test don't seem related to the changes, right? And when running the integration tests locally I get a few errors but they seem all to be related to LitPop. |
|
I do not see why the failures would be related to litpop, as the build seem to fail before the tests even start. But I agree that this looks not related. I would thus merge. |
|
Ok great, thanks! Yes, I was saying that when running the integration tests locally there is some issue with Litpop data, but maybe that is some problem due to my local setup etc |
Changes proposed in this PR:
intensity_thresattribute from Hazard class (including minor changes in some local exceedance functions that made use of this attribute).StormEuropeandTropCyclone, as it is used in data storing there. The handling of the attribute was different in both classes, which I have adapted such that both now use the handling as inTropCyclone.This PR fixes #1007.
PR Author Checklist
develop)PR Reviewer Checklist