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 all uses of Tag from climada/engine #743

Merged
merged 9 commits into from
Jul 10, 2023
Merged

Conversation

peanutfun
Copy link
Member

@peanutfun peanutfun commented Jul 4, 2023

Changes proposed in this PR:

  • Remove all uses of Tag from climada/engine/

PR Author Checklist

PR Reviewer Checklist

climada/engine/impact.py Outdated Show resolved Hide resolved
Copy link
Member

@chahank chahank left a comment

Choose a reason for hiding this comment

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

Great progress on the quest towards removing the Tag from CLIMADA. Thanks!

@peanutfun
Copy link
Member Author

peanutfun commented Jul 5, 2023

@chahank There is another issue: Impact.from_eih now has an unused argument impfset: https://github.com/CLIMADA-project/climada_python/pull/743/files#diff-1c276908f04d1a0ca5b1692295a586ce54b0937673711203582d3d72099ecdd0R210

The impfset was only used to populate the tag. I propose to deal with this the following way:

  • Remove the impfset parameter from the method immediately
  • Update all uses of from_eih within Climada (there are not many)
  • Add a type check for the second argument. If it is an ImpactFuncSet, raise an error with useful error message

This way, we are not backward compatible but users know how to fix the issue.

@chahank
Copy link
Member

chahank commented Jul 5, 2023

@chahank There is another issue: Impact.from_eih now has an unused argument impfset: https://github.com/CLIMADA-project/climada_python/pull/743/files#diff-1c276908f04d1a0ca5b1692295a586ce54b0937673711203582d3d72099ecdd0R210

The impfset was only used to populate the tag. I propose to deal with this the following way:

* Remove the `impfset` parameter from the method immediately

* Update all uses of `from_eih` within Climada (there are not many)

* Add a type check for the second argument. If it is an `ImpactFuncSet`, raise an error with useful error message

This way, we are not backward compatible but users know how to fix the issue.

Excellent idea! Although I think we do not need to make the error message. Most users will not directly use this method anyway imho.

@peanutfun
Copy link
Member Author

Alright, even easier 🙌

@chahank
Copy link
Member

chahank commented Jul 6, 2023

Oh, and probably you should go through the documentation and check whether the tag is explicitly mentioned. If yes, please remove it.

@peanutfun
Copy link
Member Author

@chahank I removed the impfset parameter from Impact.from_eih without adding an type check, as discussed. I removed the mention of Tag from the impact tutorial. It is still mentioned in other tutorials (e.g. exposures and hazard) though, but I would remove these instances once tags are removed from the respective classes.

@chahank
Copy link
Member

chahank commented Jul 10, 2023

Excellent, looks good to me!

@peanutfun
Copy link
Member Author

Thanks for the input, merging now!

@peanutfun peanutfun merged commit f0911fa into develop Jul 10, 2023
3 checks passed
@emanuel-schmid emanuel-schmid deleted the remove-tag-from-engine branch August 11, 2023 14:02
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.

3 participants