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 incorrect font for colorbar unit #6802

Merged
merged 23 commits into from
Aug 31, 2023
Merged

Fix incorrect font for colorbar unit #6802

merged 23 commits into from
Aug 31, 2023

Conversation

PaulWessel
Copy link
Member

@PaulWessel PaulWessel commented Jun 17, 2022

See #6799 for background. The label on the color bar for units was set using annotation font and offset instead of the correct label font and offset. @seisman, please build and run tests and see if you agree with me that the ~35 failing tests are better with the larger font for label and in line with other use of labels. We can then address this after 6.4 as a bug fix.

The label on the color bar for units was set using annotation font and offset instead of the correct label font and offset.
@PaulWessel PaulWessel added the bug Something isn't working label Jun 17, 2022
@PaulWessel PaulWessel added this to the Future release milestone Jun 17, 2022
@PaulWessel PaulWessel requested a review from seisman June 17, 2022 18:40
@PaulWessel PaulWessel self-assigned this Jun 17, 2022
@PaulWessel
Copy link
Member Author

Just pinging @GenericMappingTools/core as well for now.

@PaulWessel PaulWessel modified the milestones: Future release, 6.5.0 Jun 19, 2022
@seisman seisman modified the milestones: 6.4.0, 6.5.0 Jun 25, 2022
@PaulWessel
Copy link
Member Author

Have not heard back from anybody about this so pinging @seisman, @maxrjones, @joa-quim and @Esteban82. Perhaps it is worth more discussion. Here is what master currently does, using the FONT_ANNOT_PRIMARY [12p] for the y-axis label:

master

This PR would declare a bug and state that as for all basemaps the FONT_LABEL [16p] should be used instead, giving

wip

Once could argue that colorbars are very unusual basemaps in that one axis is much longer than the other (factor 20 by default) and that the short axis is never annotated or ticked. Hence, using the label font for a short unit label seems too large. This is of course why I switched to the annotation font decades ago.

However, as it is we are unable to change it independently of the axis annotation, and this WIP solution would affect both axes labels. An alternative would be to declare the bug differently and say it was meant to be the secondary annotation font FONT_ANNOT_SECONDARY [14p] instead of the primary, thus allowing for

  • A smaller default font size than label font [14p vs 16p]
  • A way to change it without affecting the annotation or main axis label settings

I must say this type of bug-fix offers a better solution for this type of plot. Thoughts?

@maxrjones
Copy link
Member

I must say this type of bug-fix offers a better solution for this type of plot

agreed, for this case it is important for the unit label to be controlled independently from the colorbar label

@joa-quim
Copy link
Member

joa-quim commented Jul 3, 2022

I don't like the bottom example. Font is way too big. But agree that a different control for the two labels (with a nice default, auto-scaled?) is useful.

@PaulWessel
Copy link
Member Author

Yes true. The whole concept of treating the tiny colormap as a basemap with the same defaults as an actual basemap is wrong. It would seem that the font sizes, ticks etc ought to scale with the color bar length, independent of the auto-scaling for the map it is attached to. Otherwise one has to do a lot of --PAR=value to make it look OK.

@WalterHFSmith
Copy link
Contributor

WalterHFSmith commented Jul 4, 2022 via email

@PaulWessel
Copy link
Member Author

Well, it was just a dumb example to show font size. Normally all my research into hair-length uses proper SI units.

@PaulWessel
Copy link
Member Author

I want to wrap this up. Sounds like we are in agreement that this is a good fix and that perhaps further work on default sizes is needed, but that is separate from this fix. Unless I hear fussing I will update those 35 PS files given this fix and complete it.

@PaulWessel
Copy link
Member Author

I am resurrecting this stale PR from last year. I decided (as I think we agreed) to use the SECONDARY annotation parameters for the little unit on the side of the bar instead of LABEL which is too big. This way, the original request in #6799 is addressed.

This will cause a bunch of failures but with gs 10 and Xcode update I have plenty already. Hoping @seisman has some clear thinking on what to do next.

@PaulWessel
Copy link
Member Author

@GenericMappingTools/core, shall we bite the bullet here and go with FONT_ANNOT_SECONDARY for the y-annotations instead of FONT_LABEL, and then deal with the tens of PostSCript files that need to be updated given the change? Otherwise this PR will sit here until nobody will remember what to do.

@joa-quim
Copy link
Member

I guess there is no escape from that (the ps updates). But, given the current ghost/gm partnership screwing of good PSs, how to tell the new failures due to this change from the others?

@PaulWessel
Copy link
Member Author

These are easy since they only affect colorers with y-label. The font size change so they all fail.

@PaulWessel
Copy link
Member Author

I have now updated the docs for colorbar to discuss FONT_ANNOT_SECONDARY instead of FONT_LABEL for the unit annotation. If you run this branch and tests you will get those extra failures that we need to update the PS for. I cannot do that (yet) because I have 100+ failures due to gs and different data versions....
When you run, do you wife server so that it downloads fresh, or how?

@PaulWessel
Copy link
Member Author

OK, we do want this fixed issue merged before 6.5. The PR allows both sides of the annotations to be different. I have noted @joa-quim's complaint that both fonts are too large. I can agree with that. In modern mode, map annotations adjust depending on map dimensions, but annotations for colorbar and presumably scales etc. are fixed and can only be adjusted with a --FONT_LABEL=... etc.

Here are two color bars, first is 15 cm the other is 3 cm. The first looks OK but the second clearly need smaller fonts (and frame pen).

15 cm:
s

3 cm:
s2

Perhaps the default font sizes should at least in modern theme like other fonts be scaled based on bar length.

To prevent backlash I think if the relevant default settings are adjusted by the user (e.g., to deal with the 3cm case) I think we leave as is, otherwise we do the default re-scaling
Give me your thoughts, @GenericMappingTools/core .

@joa-quim
Copy link
Member

Not sure I understood what you are proposing. Leaving the 3 cm example as is by default? I think it should be scaled. No reason to produce an ugly fig when we can do better.

@PaulWessel
Copy link
Member Author

I mean we should auto-scale colorbars annotations like we do for a map, so that a 3 cm colobar will have smaller annotations than the20x20 cm map it may be inserted inti. The 3c is just what we get now and it is ugly.

@joa-quim
Copy link
Member

Ah, OK we are in agreement.

@PaulWessel
Copy link
Member Author

PaulWessel commented Aug 29, 2023

Update this colorbar-unit branch, build, and run this script. It simply uses a 15 cm bar for reference and then scales all parameters down by length/15. Looks OK without having played much with "15" as the reference,

If you want to play and optimise it, see line 1006 in psscale.c and mess with that.

two

@PaulWessel
Copy link
Member Author

@Esteban82 and @anbj might also have opinions on this scaling. Basically, while gmt modern mode with modern theme automatically scales the map fonts and thickness, but we never did this for color bars. Clearly a short bar cannot just use the default primary font size (12pm) so it does need its own separate scale.

Because this will look much bette without messing with FONT_ANNOT_PRIMARY etc I think we declare a bug.

@PaulWessel
Copy link
Member Author

Maybe @seisman who initiated this issue has a comment on whether my sqrt of ratio to 15 cm looks good?

@anbj
Copy link
Contributor

anbj commented Aug 29, 2023

Do not have very strong opinions about this. All plots and scaling in #6802 (comment) looks very nice to me. So go for it?

@PaulWessel
Copy link
Member Author

@joa-quim is this an ok scaling?

@joa-quim
Copy link
Member

Yes, looks perfect.

@PaulWessel
Copy link
Member Author

Yes, looks perfect.

OK, maybe an approval?

GMT->current.setting.map_annot_offset[GMT_SECONDARY] *= scale_down;
GMT->current.setting.map_label_offset[GMT_X] *= scale_down;
GMT->current.setting.map_label_offset[GMT_Y] *= scale_down;
fprintf (stderr, "scale-down = %lg\n", scale_down);
Copy link
Member

Choose a reason for hiding this comment

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

Should remove this fprintf or change it to GMT DEBUG MESSAGE:

Suggested change
fprintf (stderr, "scale-down = %lg\n", scale_down);
fprintf (stderr, "scale-down = %lg\n", scale_down);

@seisman
Copy link
Member

seisman commented Aug 30, 2023

Update this colorbar-unit branch, build, and run this script. It simply uses a 15 cm bar for reference and then scales all parameters down by length/15. Looks OK without having played much with "15" as the reference,

If you want to play and optimise it, see line 1006 in psscale.c and mess with that.

two

Looks good to me.

@PaulWessel PaulWessel merged commit 3f4d3b1 into master Aug 31, 2023
@PaulWessel PaulWessel deleted the colorbar-unit branch August 31, 2023 11:35
@PaulWessel
Copy link
Member Author

FYI, this was merged, and the 100 new failures due to color bar font sizes etc were updated in DVC. So I am back to my 73 or so failures (mostly data related).

@joa-quim
Copy link
Member

joa-quim commented Aug 31, 2023

DVC is giving more troubles than good. I wonder if we shouldn't simply create a separate git reportory with the PS's. And when it grows too much we just delete it and restart.

C:\progs_cygw\GMTdev\gmt5\master>dvc pull
  0% Querying remote cache|                                                     |0/128 [00:00<?,    ?files/s]Exception in thread fsspecIO:
Traceback (most recent call last):
  File "threading.py", line 1016, in _bootstrap_inner
  File "threading.py", line 953, in run
  File "asyncio\base_events.py", line 603, in run_forever
  File "asyncio\base_events.py", line 1871, in _run_once
  File "selectors.py", line 324, in select
  File "selectors.py", line 315, in _select
ValueError: too many file descriptors in select()

EDIT: 5 trials later, it worked.

@joa-quim
Copy link
Member

I'm having several failures like this

equalarea_diff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants