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

Increase information encoded in virtual file names #2861

Merged
merged 9 commits into from
Mar 8, 2020
Merged

Conversation

PaulWessel
Copy link
Member

@PaulWessel PaulWessel commented Mar 8, 2020

Description of proposed changes

When memory resources are passed from one module to another, or when GMT modules are called from external interfaces (MATLAB, Julia, Python) we create a virtual file name that represents the memory container and whose encoded object ID allows us to find the memory. These names have the format @GMTAPI@-######. To prepare for a simpler scheme down the road I have extended the format of the virtual file names to @GMTAPI@-S-D-F-A-G-M-######, where the extra letters convey information about primary/secondary data, input/output, what data type, the geometry, and if a dummy messenger container for output. Currently, this information is encoded in the virtual file name but only the object ID is decoded (as before). Over time I hope to decode some of the other information for data validation.

In the process I fixed two issues: grdmath can now deal with dataset input for operators such as LDIST and INSIDE, and block*m modules can now return empty NaN grids if no data points fell inside the region. This PR closes #2476 and #2795.

Finally, I have abandoned the oldest functions and parameters used in the initial 5.2 API; This closes #2860. Three test programs have been removed since no longer testing anything useful.

All memory containers passed around as virtual files have names like @GMTAPI@-###### but this PR expands that to @GMTAPI@-#-#-#-#-##-#-######, where in addition to the 6-digit object code we also encode information like primary vs secondary input, direction, family, actual familly, geometrry, and messenger.  This will allow us to quickly learn about an object with out excessive and repetitive validations.  Also replaced GMT_STR16 with GMT_VF_LEN which is now of length 32.
Since we process the command line files separately in a program loop both g*math modules need to pass GMT_VIA_MODULE_INPUT to GMT_Read_Data.
@PaulWessel PaulWessel requested review from seisman and joa-quim and removed request for seisman March 8, 2020 05:02
@PaulWessel
Copy link
Member Author

Hope @joa-quim can give this a try with Julia for the grdmath and blockm* cases.

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

Looks like I'll have to revisit my MB built (a potential big pain). With this branch I get

julia>
[ Info: Precompiling GMT [5752ebe1-31b9-557e-87aa-f909b540aa54]
gmt [ERROR]: Unable to open GMT shared mbgmt library: Error <127>: The specified procedure could not be found.

@PaulWessel
Copy link
Member Author

Might be worth it. Some months ago we added gmt_core_module.c etc functions to report both classic and modern names. Perhaps some naming changes is behind your error.

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

I suspect more that I was using some GMT5 API thing that now is gone. I don't get that error with master branch.

@PaulWessel
Copy link
Member Author

Maybe thinks like header->nx instead of n_columns?

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

Maybe, but those should raise compiler errors.

@PaulWessel
Copy link
Member Author

True. When checking what special gmt_supplements_* function we have I find this:

nm -g supplements.so | grep gmt_supplements
00000000000063d0 T _gmt_supplements_module_classic_all
00000000000065b0 T _gmt_supplements_module_group
0000000000006510 T _gmt_supplements_module_keys
0000000000006360 T _gmt_supplements_module_list_all
0000000000006160 T _gmt_supplements_module_show_all

So I think your mb build should have the same module calls. They have changed a bit since our structure now contains both classic and modern names.

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

Well, I was not rebuilding the mb supplement. Now I'm getting many compiling errors. Let's see if it simple to clear them.

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

Seems we changed the gmt_init_module() API.It now has one more argument.

@PaulWessel
Copy link
Member Author

That was many moons ago though.

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

We removed something related to pthread in CMake scripts didn't we? I have tons of these

c:\progs_cygw\mb-system\include\iowrap-posix.h(81): fatal error C1083: Cannot open include file: 'pthread.h': No such file or directory

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

What happens is that I have this

	set (EXTRA_INCLUDE_GMTSUPPL "mb/suppl_add_mb.txt")

but now the use of EXTRA_INCLUDE_GMTSUPPL has been removed from the CMakeList and my MB build it broken.

@PaulWessel
Copy link
Member Author

Maybe @seisman can advice on the pthreads and EXTRA_INCLUDE_*, but for non-GMT shared libraries they should build separately and only link with the GMT libraries, no. E>g., see gmt-custom for building a separate shared lib the needs GMT.

@PaulWessel
Copy link
Member Author

For Linux/macoS the mbsystem installer builds the shared library, so this is a Win only thing. You should not need any Cmake stuff really, and just have a batch script or similar that does its thing?

@PaulWessel
Copy link
Member Author

BTW, I need to so some work on gmt-custom it seems.

@seisman
Copy link
Member

seisman commented Mar 8, 2020

We removed something related to pthread in CMake scripts didn't we? I have tons of these

c:\progs_cygw\mb-system\include\iowrap-posix.h(81): fatal error C1083: Cannot open include file: 'pthread.h': No such file or directory

Yes, pthread was removed in #1937. If mbsystem needs pthread, you probably need to add the cmake snippet back to the CMakeLists.txt of mbsystm.

What happens is that I have this

	set (EXTRA_INCLUDE_GMTSUPPL "mb/suppl_add_mb.txt")

but now the use of EXTRA_INCLUDE_GMTSUPPL has been removed from the CMakeList and my MB build it broken.

We changed EXTRA_INCLUDE_GMTSUPPL to EXTRA_MODULES_SUPP in #2691.

@PaulWessel
Copy link
Member Author

I think the main thing is that any supplemental requirements shall go into the setup or CMakeLists.txt for the supplemental library, not GMT itself.

@PaulWessel
Copy link
Member Author

FYI, the gmt-custom was moved to git and renamed custom-supplements. I will update it for GMT6 now.

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

The MB Win build would be an act of pure masochism if I didn't need it for classes. Everything works to break it all the time.
It doesn't go into GMT. Only to my private build of GMT.

@PaulWessel
Copy link
Member Author

OK, I will be updating custom-supplements, hopefully with @seisman's help, to make it simpler to create custom supplements. Not clear to me if this will help you or not, though.
OK for me to merge this PR?

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

No, it's not to build a custom supplement that I'm after but I made it build again the MB+GMT. Will need to test it but not now.

Meanwhile the grdmath issue became a little worse. This now crashes Julia

G = gmt("grdmath -R60/95/6/24 -I0.1 ? LDIST", mat2ds([73 24; 89 6]))

@joa-quim
Copy link
Member

joa-quim commented Mar 8, 2020

But go ahead and merge this. We'll deal with the other issue after.

@PaulWessel
Copy link
Member Author

Does work for gmt mex though. Perhaps your *.jl code needs an update since I made changes in gmt.h and gmt_resources.h, including changing the GMT_STR16 to GMT_VF_LEN 32?

@PaulWessel PaulWessel merged commit e91a8d0 into master Mar 8, 2020
@PaulWessel PaulWessel deleted the new-api-scheme branch March 8, 2020 23:53
@joa-quim
Copy link
Member

joa-quim commented Mar 9, 2020

Don't know what but this branch broke GMT.jl

@PaulWessel
Copy link
Member Author

I am pretty sure you have told me in the past that you need to make Julia changes if I change anything in gmt.h and gmt_resources.h. Did you do those?

@joa-quim
Copy link
Member

joa-quim commented Mar 9, 2020

Found it. It was the change in GMT_RESOURCE.

seisman added a commit to GenericMappingTools/pygmt that referenced this pull request Mar 11, 2020
PyGMT fails with the latest GMT master branch with the following error message:
```
GMTCLibError: Constant 'GMT_STR16' doesn't exits in libgmt.
```

To reproduce the issue, you can run:
```
import pygmt
fig = pygmt.Figure()
fig.basemap(region='0/10/0/10', projection='X10c', frame=True)
fig.plot(x=5, y=5, style='c0.2c')
```

The error is caused by the recent change of the length of virtual file names
from `GMT_STR16` to `GMT_VF_LEN` in the core GMT
(see GenericMappingTools/gmt#2861).

Changing `GMT_STR16` to `GMT_VF_LEN` is the easiest fix.
To keep compatibility with both GMT 6.0.0 and the upcomming 6.1.0,
this PR checks GMT version and use `GMT_STR16` for 6.0.0, otherwise
use `GMT_VF_LEN`.
seisman added a commit that referenced this pull request Mar 14, 2020
testapi was removed in #2861.
@seisman seisman mentioned this pull request Mar 14, 2020
seisman added a commit that referenced this pull request Mar 14, 2020
testapi was removed in #2861.
seisman added a commit to GenericMappingTools/pygmt that referenced this pull request Mar 18, 2020
….0 (#397)

PyGMT fails with the latest GMT master branch with the following error message:
```
GMTCLibError: Constant 'GMT_STR16' doesn't exits in libgmt.
```

To reproduce the issue, you can run:
```
import pygmt
fig = pygmt.Figure()
fig.basemap(region='0/10/0/10', projection='X10c', frame=True)
fig.plot(x=5, y=5, style='c0.2c')
```

The error is caused by the recent change of the length of virtual file names
from `GMT_STR16` to `GMT_VF_LEN` in the core GMT
(see GenericMappingTools/gmt#2861).

Changing `GMT_STR16` to `GMT_VF_LEN` is the easiest fix.
To keep compatibility with both GMT 6.0.0 and the upcomming 6.1.0,
this PR checks GMT version and use `GMT_STR16` for 6.0.0, otherwise
use `GMT_VF_LEN`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants