-
Notifications
You must be signed in to change notification settings - Fork 41
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
Define meaning of empty Axis.unit #2632
Define meaning of empty Axis.unit #2632
Conversation
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.
Looks good to me.
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.
For background see my comments in #2633
The following: "When axis bounds are specified by the user, on the other hand, a tool may choose a unit for the variable such that the range of the variable values (expressed in the chosen unit) fit nicely with the range of the unitless axis."
to me suggests that one can give min/max without unit and have the unit adapt to that in some way.
I may be misinterpreting it, but it should at least be clear that min/max must be given in the unit and if the unit is missing or ignored so are min/max.
When \lstinline!unit! is empty, and axis bounds are to be determined automatically, a natural choice of unit could be the variable's \lstinline!displayUnit!. When axis bounds are specified by the | ||
user, on the other hand, a tool may choose a unit for the variable such that the range of the variable values (expressed in the chosen unit) fit nicely with the range of the unitless axis. |
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.
Quoting @HansOlsson regarding these lines, so that we get a subthread for discussion:
The following: "When axis bounds are specified by the user, on the other hand, a tool may choose a unit for the variable such that the range of the variable values (expressed in the chosen unit) fit nicely with the range of the unitless axis."
to me suggests that one can give min/max without unit and have the unit adapt to that in some way.I may be misinterpreting it, but it should at least be clear that min/max must be given in the unit and if the unit is missing or ignored so are min/max.
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.
to me suggests that one can give min/max without unit and have the unit adapt to that in some way.
No. The of the unitless axis at the end of the sentence is there to make it clear that the axis still doesn't have a unit; the units are always associated with the variables when unit
is empty. It is true that it becomes up to the tool to make a clever choice of unit when min
and max
are given, but unit
is empty; a bad choice will result in a poor fit between the variable values and the axis range.
I may be misinterpreting it, but it should at least be clear that min/max must be given in the unit and if the unit is missing or ignored so are min/max.
No. A bad unit is an error, but there is still a suggested way to deal with it. More importantly, min
and max
do have a well defined meaning when unit
is empty, as described above. That min
and max
are given in unit
is expressed by the pseudocode record, expressed in one way in this PR, but more clearly in #2633:
record Axis
Real min "Axis lower bound, in 'unit'";
Real max "Axis upper bound, in 'unit'";
String unit "Unit of axis tick labels";
String label "Axis label";
end Axis;
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.
My suggestion is that if we don't recognize the unit, it's incorrect, or if it is empty we ignore the min and max as well.
The tools must be able to handle missing unit and min/max - so just fall back to that instead of having advanced logic to handle such problems. It seems simpler and gives more predictable result, and it encourages users to either specify the plots in detail - or not at all.
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 don't see the reason to ignore min
and max
when unit
is empty (there is no complicated logic involved in making use of them). The possibility to have variables of different units on the same axis is quite useful, and you get this mode by using an empty unit
. In this mode, it is still perfectly meaningful to specify min
and max
boundaries for the unitless axis.
Something I think we could clarify, however, is the recommendation for what to do when unit
is not recognized: Then, the min
and max
should be ignored (instead of being applied to the unitless axis) — I'll make a separate comment for this.
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.
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.
If we want to plot variables in different units with given min/max the most reasonable would be to say that they use the given unit (or displayUnit) of the variables; nothing fancy. If I for the curve above gave min=0, max=0.4 it's not clear if the tool should use "V" or "kV" - and to me it seems as if the specification recommends "kV" (and "kW") since more of the curve is shown in that case.
The other option would be something a lot more advanced (allow different scalings).
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.
Your interpretation of the recommendation is in line with the intention, and I think we need to stick to mappings to the axis range that can be expressed by the choice of unit.
I would prefer to not rely directly on the variable's displayUnit
here, since the displayUnit
was probably set with just the variable at hand in mind, not how the range of the variable would match the ranges of some other variable when added to the same unitless plot axis. If you don't like the tool-dependent choice of unit here, the compromise that comes to mind is to extend Curve
with (optional) xDisplayUnit
and yDisplayUnit
, which would default to the displayUnit
of the x
and y
, and then require that xDisplayUnit
and yDisplayUnit
are used on unitless axes.
My main objection to introducing xDisplayUnit
and yDisplayUnit
is that it could be confusing that these only come into play when plotted against a unitless axis; the amount of added value compared to added specification text (after adding clarifications to avoid the potential confusion) isn't that great. The other objection is that introducing xDisplayUnit
and yDisplayUnit
would mean that tools don't get the chance to make an intelligent choice of unit as in the current proposal.
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.
If we want to plot variables in different units with given min/max the most reasonable would be to say that they use the given unit (or displayUnit) of the variables; nothing fancy.
But the wording doesn't require you to do anything fancy, it only gives you enough leeway to do fancy stuff if you want to.
This PR tries to address the problem that it is currently not allowed to leave the
unit
of anAxis
empty, which in turn must be interpreted as also the axis specificationsx
andy
of aPlot
being mandatory. I don't think it was intended to add the burden of specifying axes for everyPlot
, and specifying that it means to leave theunit
empty opens up for an interesting feature currently not present in the MCP.This PR is a follow-up to #2579.