-
Notifications
You must be signed in to change notification settings - Fork 276
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
BUG: Set the display name for new fields #3282
Conversation
It looks like I broke a bunch of answer tests with this change. The main failing culprits are |
sounds about right. I'd argue that inheriting the display name is actually what we want, and "Cell Mass" makes more sense than "Mass" as a label, in my opinion. Unless I'm missing an argument here, I think the answers should actually be updated. |
In general what we want to do is avoid referencing the underlying data structures (cell or particle) and focus on the physical quantities so as to make scripts as portable as possible across different frontends. In most use cases there's no reason why a user shouldn't be able to take a script written for Gadget and use it for Enzo, etc. or vice-versa. |
Ah right, this is a good reason. I'm guessing we currently don't support aliases with alternate reprs (display names)? |
I thought we did? In the field tuples there's the option for that.
…On Thu, May 20, 2021 at 8:08 AM Clément Robert ***@***.***> wrote:
Ah right, this is a good reason. I'm guessing we currently don't support aliases with alternate reprs (display names)?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
I agree with @jzuhone that we don't want Any ideas anyone has on where I could appropriately set the |
Kinda seems like this would be a pretty big change at this point in the
game?
…On Sat, May 22, 2021 at 7:25 PM Cameron Hummels ***@***.***> wrote:
I agree with @jzuhone <https://github.com/jzuhone> that we don't want
cell_mass in the PhasePlot zlabel when it comes from a ('gas', 'mass')
field, even when that is aliased in some cases to ('FRONTEND',
'cell_mass').
Any ideas anyone has on where I could appropriately set the display_name
to avoid this problem? The way it works with ProjectionPlot and SlicePlot
is in FixedResolutionBuffer right when the plot is being generated. But
I'm not sure where to make that happen for ProfilePlots and PhasePlots--I
guess they don't rely on an FRB for those plots.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3282 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXOYWMJK7HO3LOXHDV2LTPBDODANCNFSM45FZM75Q>
.
|
@matthewturk can you elaborate a bit? |
I guess just thinking about changing "Cell Mass" in the z label of
plots to something different struck me as a big change.
…On Sat, May 22, 2021 at 8:13 PM John ZuHone ***@***.***> wrote:
@matthewturk can you elaborate a bit?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
If you're plotting the |
I think there's some confusion here, potentially because of the long string of comments, so I'll try to summarize. The existing behavior in yt for a PhasePlot weighted by the So PhasePlot/ProfilePlot doesn't create an FRB. I guess I can just try to set the label in those functions directly. |
So I rewrote this to set the display_field in If people are OK with this solution, I will make some tests and update the answer tests. |
yt/visualization/profile_plotter.py
Outdated
elif field_name.find("$") == -1: | ||
field_name = field_name.replace(" ", r"\ ") | ||
field_name = r"$\rm{" + field_name + r"}$" | ||
if fractional: | ||
label = field_name + r"$\rm{\ Probability\ Density}$" | ||
elif field_unit is None or field_unit == "": | ||
label = field_name | ||
elif "frac" in field_unit: |
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.
Same here
Good call, @cphyc . I think the code is now stable against any fields that might have So this should be ready to go, I think. In short, it corrects the problem described in Issue #3269. It also corrects some issues with parentheses in field units displayed in PhasePlot (the parentheses were too small to cover fractional units like g/cm**3). See the image above in the comments. I guess I need to generate new answer tests for this? |
@chummels your latest change is causing image comparison tests to fail, but the change is actually desirable ! so you actually fixed bad latex here, thank you ! edit: neglected to read the conversation history before adding this comment. So of course your change is 100% intentional and that's great. Sorry for suggesting it was a happy incident. |
Sure that sounds fine. We can get all these label issues fixed one at a time. |
#3520 is in. Now we need to bump the answer store for this PR. Let's run the answer tests again to get an up to date result we can submit to the answer store |
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.
@chummels be aware that matplotlib 3.5 is around the corner (https://github.com/matplotlib/matplotlib/milestone/59) and may cause perturbations with the answer store, so I'd suggest to either expedite this PR (with a companion PR to the answer store) or hold it until after matplotlib 3.5 is released. Let me know if you need a hand, the process still takes a little amount of human overhead :)
I'm happy either way, whichever is easiest. I'm happy to make a companion PR to the answer store, but I'm not sure how to do this. I'd love your assistance with it since you offered. |
Alright so here are the steps:
Admittedly this is hard to guess, but I think you have everything you need here. Don't hesitate to reach out in case something isn't clear. |
Co-authored-by: Corentin Cadiou <contact@cphyc.me>
25ae645
to
746d516
Compare
Ah, answer tests failed again. Apparently we needed to rebase this branch ahead of regenerating images for it to take into account the changes from #3520 . I rebased the branch here and produced a new companion PR for the answer store yt-project/answer-store#25 |
@meeseeksdev backport to yt-4.0.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove If these instruction are inaccurate, feel free to suggest an improvement. |
Let's try this one last time now that a lot of other PRs went in. If it fails we'll resort to manual backport |
BUG: Set the display name for new fields
PR Summary
This PR addresses Issue #3269. If a new derived field has not defined a
display_name
(used for rendering in figures) at creation time, this generates it at the end of initialization. This probably has little impact on most fields being generated, but this will ensure that species fields (e.g., H_p0 =H I
) are rendering correctly in bothPhasePlots
andProfilePlots
instead of just inProjectionPlots
andSlicePlots
. Example (seez_label
):