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

Feature/coverage #616

Merged
merged 14 commits into from
Jan 16, 2023
Merged

Feature/coverage #616

merged 14 commits into from
Jan 16, 2023

Conversation

NicolasColombi
Copy link
Collaborator

@NicolasColombi NicolasColombi commented Jan 9, 2023

Changes proposed in this PR:

Add test for all the plotting functions of
climada/engine/cost_benefit.py listed below.

1)plot_cost_benefit
2)plot_event_view
3)plot_waterfall_accumulated
4)plot_waterfall
5)plot_arrow_averted
6)_plot_list_cost_ben

PR Author Checklist

PR Reviewer Checklist

NicolasColombi and others added 11 commits December 9, 2022 10:34
Add 3 test to check that the plotting functions of
climada/entity/exposure/base.py do not throw
any errors when they are called. Specifically, the
functions tested are:

1) plot_scatter
2) plot_raster
3) plot_basemap
Add test that check that the function plot_rp_intensity(), present in
climada/hazard/base.py, do not throw errors when it is called.
Add test to check that plot() in climada/entity/disc_rates/base.py
do not raise errors when called.
1) Add test for centroid plots.
2) Add a test to check that the right title is displayed when using
plot_rp_intensity() to plot hazard exceedance intensity
Add test for all the plotting functions of
climada/engine/cost_benefit.py listed below.

1) plot_cost_benefit
2) plot_event_view
3) plot_waterfall_accumulated
4) plot_waterfall
5) plot_arrow_averted
6) _plot_list_cost_ben
Comment on lines 31 to 39
<<<<<<< HEAD
from climada.entity.entity_def import Entity
from climada.entity.exposures.base import Exposures
from climada.entity import DiscRates,ImpfTropCyclone
from climada.entity.measures import Measure, MeasureSet
=======
from climada.entity.exposures.base import Exposures
from climada.entity import DiscRates
>>>>>>> develop
Copy link
Member

Choose a reason for hiding this comment

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

These are leftovers from a merge conflict. The file cannot be executed in this state. Please "merge" the lines above the ==== and below, and remove the <<<< / >>>> markers. See "Basic Merge Conflicts" in Git Book: 3.2 Git Branching - Basic Branching and Merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes, my bad, I must have forgotten them. I made a new commit that should fix the left overs and convert docstring comments to regular comments with #. I hardly see comments on the code, should I also not comment ? I just thought a few comments might be helpful in structuring the long code to create a costbenefit object.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! Generally, adding comments and using them to structure your code is important and recommended. However, there is a tradeoff when it comes to adding comments where there are no changes related to the PR. If you already worked through the class and think the code might need more comments, I think it would be best to add them in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy that! I was referring mainly to the comments in the code that I added so I won't need to open a new PR to comment the class in general, but I will keep that in mind thanks!

NicolasColombi and others added 3 commits January 13, 2023 15:34
1) Fix merging conflicts
2) Replace docstrings comments with regular comments
@emanuel-schmid
Copy link
Collaborator

Cool. Thanks! 😄
For the next time: try to keep the imports as simple as possible, one of your commits caused the linter to raise a recursion error (at least in the version we use).
More generally, although it's just a test file and thus excluded from pylint checks, please try to apply to the basic linter rules nonetheless.

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.

Good job! These plots are getting more complex to set up but you seem to get the hang of CLIMADA 😉

haz_future, entity_future)
costben.plot_arrow_averted(axis = ax, in_meas_names=['Measure A', 'Measure B'],
accumulate=True)
CostBenefit._plot_list_cost_ben(cb_list = [costben])
Copy link
Member

Choose a reason for hiding this comment

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

_plot_list_cost_ben is a private method that is already called by plot_cost_benefit. I should not be necessary to call it explicitly to increase the coverage.

Suggested change
CostBenefit._plot_list_cost_ben(cb_list = [costben])

@emanuel-schmid emanuel-schmid merged commit c958295 into develop Jan 16, 2023
@emanuel-schmid
Copy link
Collaborator

🙌

@emanuel-schmid emanuel-schmid deleted the feature/coverage branch January 16, 2023 14:13
@NicolasColombi
Copy link
Collaborator Author

Good job! These plots are getting more complex to set up but you seem to get the hang of CLIMADA 😉

Thank you!! :) @peanutfun

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