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 #5524: Use large, but finite, values for contour extensions #5546

Merged
merged 1 commit into from Dec 6, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Nov 23, 2015

This follows @efiring's suggestion in #5524.

@mdboom mdboom added this to the Critical bugfix release (1.5.1) milestone Nov 23, 2015
@tacaswell
Copy link
Member

I did a bit of digging into the git logs last night and the value used to be pm 1e300.

@mdboom
Copy link
Member Author

mdboom commented Nov 23, 2015

I have to admit I don't fully understand this code path. As long as 1e300 when normed doesn't become infinite (since that's awfully close to the limit of double precision at 1e301) than going back to the old number seems like the safest bet.

@WeatherGod
Copy link
Member

stupid question, but do we have any indication why we stopped using the old
numbers? Just want to make sure we aren't breaking something else that we
have forgotten.

On Mon, Nov 23, 2015 at 11:38 AM, Michael Droettboom <
notifications@github.com> wrote:

I have to admit I don't fully understand this code path. As long as 1e300
when normed doesn't become infinite (since that's awfully close to the
limit of double precision at 1e301) than going back to the old number seems
like the safest bet.


Reply to this email directly or view it on GitHub
#5546 (comment)
.

@efiring
Copy link
Member

efiring commented Nov 23, 2015

Good question; it turns out I'm the guilty party: #1022. I don't see any direct motivation there other than the same one that prompted all the attempts here to use finfo etc.: it seemed like the most general solution. And it was, until the (reasonable) change to pcolormesh interfered with it.
@mdboom, yes, 1e300 is too close to the limit, which I was misremembering as 1e308. To maximize the range of norming possibilities, 1e150 would seem about right.
I suppose an alternative would be some additional code (maybe with a private kwarg) in pcolormesh to handle this special case. I suspect that some changes in the Normalize classes might also be involved.
Maybe the thing to do is use 1e150 now and open a new issue as a reminder to revisit the whole thing. It seems a shame to get too hung up right now over rare potential use cases that would fail with 1e150.

@tacaswell
Copy link
Member

How about the average of vmin/vmax (the local one) and the very big number? That way if the user is trying to contour huge values we will still pull the extend values past them.

Anything we do here should probably also be applied to https://github.com/matplotlib/matplotlib/pull/5546/files#diff-2e46ac77d349b367229eda1b93d92fc3L1220 where the limits of the extra contours are set up.

I might be misunderstanding, but I think both of these if statements could just be removed due as the average of the pulled down min should be less than the min used for norming.

@mdboom
Copy link
Member Author

mdboom commented Dec 6, 2015

@efiring: I think you may have commented on @tacaswell's last comment here in a hangout. But just for completeness, can you say what you think we should do here. I'm fine with merging this as is for now so we can move forward with testing -- it fixes a real regression, and I don't think it makes things any worse than originally. There's probably a better/cleaner solution to be found, but I'm not sure we want that to hold us up.

@efiring
Copy link
Member

efiring commented Dec 6, 2015

Regarding the three parts of @tacaswell's comment:

  1. Yes, something like this could work as well, making it more likely (but not guaranteed) that a correct result would be obtained with numbers exceeding the present proposal (fixed value of +- 1e150). It's slightly more complex, and harder to say when it will work and when it won't; so I still prefer the simple limits for now.
  2. Nothing needs to be changed on Line 1220; it works perfectly as-is. It is not as closely related to this PR as it might appear.
  3. I don't understand the comment.

efiring added a commit that referenced this pull request Dec 6, 2015
Fix #5524: Use large, but finite, values for contour extensions
@efiring efiring merged commit ca892ea into matplotlib:master Dec 6, 2015
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.

None yet

4 participants