Skip to content

Get tighter w/e/s/n domain for general perspective projection#5776

Merged
PaulWessel merged 3 commits intomasterfrom
improved-genper-search
Sep 18, 2021
Merged

Get tighter w/e/s/n domain for general perspective projection#5776
PaulWessel merged 3 commits intomasterfrom
improved-genper-search

Conversation

@PaulWessel
Copy link
Member

For various reasons we need to determine the encompassing w/e/s/n rectangle in degrees of a map for all projection settings, such as for calling gmt_map_setup or finding grid bounds. For the general perspective projection this is complicated by the fact that for some projection parameters the map window is the intersection of a rectangle and a sphere, yielding an odd rounded octagon. We then correctly searched along this perimeter and determined the extreme longitude/latitude values. However, should any of the geographic poles be inside this polygon then we must set w = 0, e = 360 and update the max or min latitude. That last step did not work for the genper because we (the royal we) had lazily just copied/pasted what was done for other azimuthal projections. This PR implements this correctly by determining of those pole points is inside the spherical polygon clip path. All tests pass.

So not a significant bug in that nothing breaks, but this should reduce the number of remote tiles needed when blending together a grid for a region when using this projection, thus speeding things up. I noticed this when doing the testing of #5753 (as did @Esteban82 for the nigh/day plotting) that we were getting too crude images in the early stages. That is when the old calculations returned 0/360 when in fact a much tighter limit on longitudes were correct.

For various reasons we need to determine the encompassing w/e/s/n rectangle in degrees for all proejction settings.  For the general perspective projection this is complicated by the fact that for some projection parameters the map window is the intersection of a rectangle and a sphere, yielding an odd rounded octagon.  We then search along this perimeter and determine the extreme longitude/latitude values. However, should any of the geographic poles be inside this polygon then we just set w = 0, e = 360 and update the max or min latitude.  That last step did not work for the genper because we ahd just copied/pasted what is done for other azimuthal projections.  This PR implements this correctly by determining of those pole points is inside the spherical polygon clip path.
@PaulWessel PaulWessel added the maintenance Boring but important stuff for the core devs label Sep 17, 2021
@PaulWessel PaulWessel self-assigned this Sep 17, 2021
@PaulWessel
Copy link
Member Author

Anyone having a look at this one?

@Esteban82
Copy link
Member

Anyone having a look at this one?

Soon

@Esteban82
Copy link
Member

Is there any test? Or should approve it directly?

@PaulWessel
Copy link
Member Author

No tests since it is just an internal improvement. If you have time to run all the tests (cd to build, run make -j<cores> check [or ninja -j<cores> check] to make sure nothing funny happens on Linux.

Copy link
Member

@Esteban82 Esteban82 left a comment

Choose a reason for hiding this comment

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

I try it. Nothing funny but I not sure if I do it well.

@PaulWessel
Copy link
Member Author

Does your test command end with something like this:

...
1100/1101 Test #1050: test/spotter/spotter_09.sh ...................   Passed   11.87 sec
1101/1101 Test #1097: test/x2sys/x2sys_04.sh .......................   Passed   16.35 sec

100% tests passed, 0 tests failed out of 1101

Total Test time (real) = 190.01 sec

@PaulWessel
Copy link
Member Author

FYI, to be able to build and run the tests you need to set things in cmake/ConfigUserAdvanced.cmake, e.g.

#
# Testing and development
#

# Enable running examples/tests with "ctest" or "make check" (out-of-source).
# Need to set either DO_EXAMPLES, DO_TESTS or both and uncomment the following
# line.
enable_testing()
set (DO_EXAMPLES TRUE)
set (DO_TESTS TRUE)
set (DO_ANIMATIONS TRUE)
# Number of parallel test jobs with "make check":
set (N_TEST_JOBS 20)

set (DO_API_TESTS ON)

otherwise the check stuff does not work I think.

@Esteban82
Copy link
Member

I did it wrong. I will have to edit the UserAdvance.

@PaulWessel
Copy link
Member Author

No worries, it is a bit unclear - let me know if you run into any problems.

@Esteban82
Copy link
Member

I got this:

99% tests passed, 3 tests failed out of 1098

Total Test time (real) = 628.07 sec

The following tests FAILED:
	354 - test/gmtmex/WL_example_3.sh (Failed)
	567 - test/img/imgtrack.sh (Failed)
	595 - test/modern/longbasemap.sh (Failed)
Errors while running CTest
make[3]: *** [CMakeFiles/check.dir/build.make:57: CMakeFiles/check] Error 8
make[2]: *** [CMakeFiles/Makefile2:652: CMakeFiles/check.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:659: CMakeFiles/check.dir/rule] Error 2

@PaulWessel
Copy link
Member Author

Great, I think imgtrack.sh requires the img file to be downloaded, while longbasemap.sh definitively requires some more cmake settings. I have

# Uncomment these two statements if you are a developer debugging GMT:
add_definitions(-DDEBUG)
add_definitions(-DMEMDEBUG) # Turn on memory tracking see gmt_support.c for extra info
add_definitions(-DUSE_COMMON_LONG_OPTIONS) 	# Turn on testing of upcoming long-option syntax for common GMT options
add_definitions(-DUSE_MODULE_LONG_OPTIONS) 	# Turn on testing of upcoming long-option syntax for module options

You can look in rbuild/Testing/Temporary/LastTest.log and search for those three and see what it says.

@PaulWessel
Copy link
Member Author

Since @federico's three failures are not related to -JG I will merge this PR.

@PaulWessel PaulWessel merged commit 775c146 into master Sep 18, 2021
@PaulWessel PaulWessel deleted the improved-genper-search branch September 18, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Boring but important stuff for the core devs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants