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

remove tags from hazard classes #88

Merged
merged 6 commits into from
Aug 25, 2023
Merged

Conversation

emanuel-schmid
Copy link
Contributor

@emanuel-schmid emanuel-schmid commented Aug 8, 2023

Changes proposed in this PR:

  • Remove anything dealing with tags from the hazard classes of climada_petals.
    This is part of the overall climada refactoring to remove the Tag classes.

This PR fixes #

PR Author Checklist

PR Reviewer Checklist

@peanutfun peanutfun mentioned this pull request Aug 21, 2023
13 tasks
Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I only suggest to improve the Changelog entry.

@@ -40,7 +40,6 @@
from climada.hazard.base import Hazard
from climada.hazard.centroids import Centroids
import climada.util.coordinates as u_coord
from climada.util.tag import Tag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this work before? I don't see a climada/util/tag.py module in any of the recent releases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after release 3.3, hazard.tag and entity.tag have been merged

Comment on lines -280 to -283
yearrange=TARGET_YEARRANGE, yearrange_ref=REFERENCE_YEARRANGE,
gh_model=None, cl_model=None,
scenario='historical', scenario_ref='historical', soc='histsoc',
soc_ref='histsoc', fn_str_var=FN_STR_VAR, keep_dis_data=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, that all of this information was only put into the tag 🤔

CHANGELOG.md Outdated
Comment on lines 20 to 21
- several adaptations have been necessary because of the `Tag` class being removed from the CLIMADA core package:
[#88](https://github.com/CLIMADA-project/climada_petals/pull/88)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to put this into the "Removed" category:

### Removed

- `tag` attribute from hazard classes [#88](https://github.com/CLIMADA-project/climada_petals/pull/88)

@emanuel-schmid
Copy link
Contributor Author

🙌 thanks for the review!

@emanuel-schmid emanuel-schmid merged commit ccb0314 into develop Aug 25, 2023
3 checks passed
@emanuel-schmid emanuel-schmid deleted the feature/remove_tag_hazard branch August 25, 2023 14:03
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

Successfully merging this pull request may close these issues.

2 participants