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

fix small density-bug and added test #189

Merged
merged 6 commits into from
May 11, 2023
Merged

Conversation

daniel-caichac-DHI
Copy link
Collaborator

@daniel-caichac-DHI daniel-caichac-DHI commented Apr 27, 2023

Hi,

  • I added an additional test for the scatter plot with density (actually extended a test by adding two lines, one with bins=19 and another with bins=21)
  • The reason behind this is that sometimes, as the way the 2d-histogram for scatter density plot was defined, given certain bin-sizes, the edges of the bins (2DHist bins) ended just before the center of the bins, which crashed the plots. This could easily be solved by changing the bin-size slightly or number of bins by one, but it is not ideal.
  • So now, I check the last value of the center of the bins, and the last value of the edge of the bins, and if the latter is smaller than the former, I add an extra 1/2 bin, to make sure that the bin-edges fully cover the bins-center. This has no impact on the final result (plot), but avoids the error.

fmskill/plot.py Outdated Show resolved Hide resolved
@daniel-caichac-DHI daniel-caichac-DHI removed the request for review from jsmariegaard April 27, 2023 08:34
@@ -552,6 +557,30 @@ def _scatter_density(x, y, binsize: float = 0.1, method: str = "linear"):
for i in range(len(cxy)):
Copy link
Member

Choose a reason for hiding this comment

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

Is the double loop necessary?

How about?

hist = histodata.flatten()

Copy link
Collaborator Author

@daniel-caichac-DHI daniel-caichac-DHI May 8, 2023

Choose a reason for hiding this comment

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

I do not get the same result if I do that, also some tests fail. Double loop works :)
Also the histogram looks flat for some reason (no pun intended) as in, with the double loop I get this (see high density cloud in light colors):
image
but with the flattened histogram it just looks dark everywhere
image

fmskill/plot.py Outdated Show resolved Hide resolved
Copy link
Member

@jsmariegaard jsmariegaard left a comment

Choose a reason for hiding this comment

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

Great. Thanks

Why do you use double-underscore for prefixing your helper functions? - I guess they are not that secret.

Also, would good with removing the double loop but it is not essential in my eyes

@daniel-caichac-DHI
Copy link
Collaborator Author

Great. Thanks

Why do you use double-underscore for prefixing your helper functions? - I guess they are not that secret.

Also, would good with removing the double loop but it is not essential in my eyes

Because henrik suggested making them private, I guess
#189 (comment)

@daniel-caichac-DHI daniel-caichac-DHI merged commit 3138331 into main May 11, 2023
@jsmariegaard jsmariegaard deleted the fix_density_bins_bug branch November 20, 2023 08:42
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