-
Notifications
You must be signed in to change notification settings - Fork 280
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 an unintuitive behaviour in ProfilePlot #2650
Merged
munkm
merged 7 commits into
yt-project:master
from
neutrinoceros:simplify_profileplot_api
Jul 1, 2020
Merged
fix an unintuitive behaviour in ProfilePlot #2650
munkm
merged 7 commits into
yt-project:master
from
neutrinoceros:simplify_profileplot_api
Jul 1, 2020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…was equivalent to x_log=True
neutrinoceros
added
the
api-consistency
naming conventions, code deduplication, informative error messages, code smells...
label
Jun 19, 2020
…pplicated sanitizing code.
neutrinoceros
force-pushed
the
simplify_profileplot_api
branch
from
June 19, 2020 12:41
1f4efc1
to
585f4ee
Compare
neutrinoceros
force-pushed
the
simplify_profileplot_api
branch
from
June 20, 2020 09:15
ad7fefe
to
45c0988
Compare
neutrinoceros
force-pushed
the
simplify_profileplot_api
branch
from
June 20, 2020 09:15
45c0988
to
8d57751
Compare
matthewturk
approved these changes
Jun 26, 2020
munkm
reviewed
Jun 30, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments! But this is a good idea and I agree the way that we're currently that using None for these log fields is confusing. We need to be really clear in the release notes that this may cause different behavior in scripts of our downstream users.
munkm
approved these changes
Jun 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
api-consistency
naming conventions, code deduplication, informative error messages, code smells...
enhancement
Making something better
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
Because of the way it's currently implemented,
ProfilePlot
produces unintuitive results when passedx_log=None
, which is effectively, equivalent has passingx_log=True
despite the fact thatbool(None)==False
.with this change, the following produce the same result
This issue was reported on slack by Jack Jenkins.
I'm opening this as a draft since there may be other appropriate changes to ProfilePlot.init() that could go with it.