Conversation
…climada_python into feature/tag_refactoring
…tag_refactoring # Conflicts: # climada/hazard/test/test_base.py # climada/hazard/test/test_storm_europe.py
# Conflicts: # climada/entity/exposures/base.py # climada/entity/exposures/litpop/litpop.py
chahank
left a comment
chahank
left a comment
There was a problem hiding this comment.
Great improvement toward the consolidation of the tag classes. Thanks!
climada/engine/impact.py
Outdated
| @@ -45,7 +45,6 @@ | |||
| from rasterio.crs import CRS as rasterioCRS # pylint: disable=no-name-in-module | |||
|
|
|||
| from climada.entity import Exposures, Tag | |||
There was a problem hiding this comment.
| from climada.entity import Exposures, Tag | |
| from climada.entity import Exposures | |
| from climada.util.tag import Tag |
Should it not be like this?
climada/hazard/trop_cyclone.py
Outdated
| haz.frequency_from_tracks(tracks.data) | ||
| haz.tag.description = description | ||
| haz.tag.append(Tag(description=description)) | ||
| print('tag tag tag ', haz.tag) |
There was a problem hiding this comment.
| print('tag tag tag ', haz.tag) |
| return list_of_str | ||
|
|
||
|
|
||
| class Tag(): |
There was a problem hiding this comment.
In another file, there is an equality comparison for tags. Should then the __eq__ not be defined?
There was a problem hiding this comment.
Good point! And actually yes, Tag is just the kind of object/class where implementing __eq__ would make a lot of sense, imho.
However - since Tag is to be removed altogether, I guess the effort is not justified.
(The effort of implementing __eq__ may be small enough but finding the places where it can or should be used seems to be serious work.)
There was a problem hiding this comment.
I fully agree. I would just remove the equality comparison.
There was a problem hiding this comment.
😲 Er - which equality comparison?
There was a problem hiding this comment.
I thought I had seen a tag equality comparison in one file last time I did the review. Unfortunately, I did not write down where, so maybe I was mistaken.
There was a problem hiding this comment.
I superficially looked for one but couldn't find it. Shall we remove it when we stumble across?
2 participants
Changes proposed in this PR:
haz_typeout ofTagand move it toHazardentity.Tagandhazard.Tagtoutil.TagThis PR paves the ground for the eventual removal of the
tagattributes from all climada classes.PR Author Checklist
develop)PR Reviewer Checklist