-
Notifications
You must be signed in to change notification settings - Fork 365
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
Save any default CPT for remote datasets in hidden grid header #6178
Conversation
This addresses #6167 when we cut a subset from, sa, earth_relief_10m and then no longer know what the CPT should be.
src/gmt_nc.c
Outdated
@@ -1029,6 +1032,8 @@ GMT_LOCAL int gmtnc_grd_info (struct GMT_CTRL *GMT, struct GMT_GRID_HEADER *head | |||
|
|||
/* Define z variable. Attempt to remove "scale_factor" or "add_offset" when no longer needed */ | |||
gmtnc_put_units (ncid, z_id, header->z_units); | |||
if (GMT->parent->remote_info && GMT->parent->remote_id != GMT_NOTSET && GMT->parent->remote_info[GMT->parent->remote_id].CPT[0] != '-') /* Subset of remote grid with default CPT, save name as an attribute */ | |||
gmt_M_err_trap (nc_put_att_text (ncid, z_id, "cpt", strlen (GMT->parent->remote_info[GMT->parent->remote_id].CPT), GMT->parent->remote_info[GMT->parent->remote_id].CPT)); |
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.
"cpt" is a very GMT parlance. I would prefer "palette" like those grids from OceanColor
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 mind calling it palette, but since it is just a string with a GMT CPT name it is still very GMT centric. We are not sticking a 3xn array into the grid, just a string. Still want to rename it?
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.
Ok, sorry. I thought that it was the cpt that was added to the nc file and that would be an interesting addition. If it's just a cpt name suggestion for GMT scripts then I'm not even sure we should be doing 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.
Nothing to do with GMT scripts though. This is for the module and hence for wrappers as well if interested. Since I think you pass wrapped GMT grids in Julia then this will work for you off the bat. PyGMT will need to do some other work since they read in a netcdf grid via Xarrays, so they have access to the attributes that way.
We could of course stick a CPT into the netCDF but it has little value in GMT (we would do it because the outside world wants it) and it is limiting in that 3xn matrices do not tell us continuous or discrete and now we have to add more stuff to explain that thing.
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 am not exploring the hidden struct because there is no guaranties that it won't change and then each change would mean a Julia breakage. I've pulled out some hairs in the past trying to inventing a system where the structs could have some self-referencing system that could be used to find the struct members of interest regardless of structs changes. It would involve the use of offsetof
C macro. But never came to get any workable prototype.
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.
Yes, if you do not pass any default CPT but let GMT control the default CPT then it would work. Are you supplying a different default CPT than GMT would normally (turbo?). if not then just don't supply one.
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 think this is a worthwhile improvement for the CLI, regardless of usability for the wrappers. While PyGMT can easily access the cpt attr, I'm not sure how it would communicate it back through the API during the GMT_Create_Data
, GMT_Put_Matrix
, GMT_Open_VirtualFile
process.
GMT_Set_Comment is only for the 'top-level' grid header (those listed here), correct? Could anything be added to the API for setting the items that are mostly used internally (i.e., full list here)?
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 think that is the step you will want to intervene. If you learn there is a cpt attribute then you add "-C" to the string you prepare for the module, no?
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.
Yes, we can do that. If that is the case, we may want to eventually document that users can add a cpt
attr to their netcdf grid in order to associate a default colormap with their own grids. In which case Joaquim's comment about using palette
over cpt
becomes relevant again.
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 suppose grdedit is the module where it makes sense for us to add an attribute to the grid file. Something like
grdedit my.grd -Qelevation
This brings up the issue of "I want my specially made cpt file to be embedded in the grid", to which I suggest someone read the gmtmex_parser.c on the gymnastics on converting a very rich CPT standard into a very poor Matlab color table scheme. It would be the same issue - lots of work to accommodate all the special things in a CPT, vs just adding the name of the cpt.
gmt grdimage @earth_relief_01d_g -R0/10/0/10 -B -c | ||
gmt grdimage test.nc -B -c | ||
gmt subplot end | ||
gmt end show |
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.
Here we are again creating a new PS. Preferably the test should detect the presence of the cpt but if that's too complicated, at least make the test a tinny figure.
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 just updated to remove the boundary. It is a 10x10 grid - that is pretty small, no? The size of the PS is 22K, og which 20K is the PSL header.
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 do not see how I can make a non-plot test to detect that GMT correctly identifies the attribute and stretches the CPT, etc.
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 like my idea though: It complements the attribute idea since that solution will survive a session whereas the current CPT has a limited scope in where it is found (figure, subplot, inset), etc.
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.
The current implementation is fine to me.
A slight issue has arisen: We have test and doc scripts that sets a default CPT via makecpt, then cuts a subset via grdcut afterwards. Now, that is changing the current CPT. In some places I can switch the order of those to commands, but i others we set a current cpt before a subplot and then we have grdcut commands inside those panels and now the CPT takes on the one in the grid since the grdcut command forces the resetting of the current CPT. Two options:
Any thoughts on this, @joa-quim and @meghanrjones? Note that GMT modules etc has no need for the grdcut feature so I am OK with option 1, but willing to do option 2 if there is any desire to keep this feature for the externals. |
Option 1 would still leave the cpt attr and use that as the default if there isn't already a colormap set in the session, correct? If so, I think that's fine. |
Yes, the attribute in the written grid remains, it is just the whole section
that will be rescinded due to unintended consequences... |
Sounds fine. |
Yes ls agree to let it there to be used if expressly stated and not a setting that happens by default. |
Could you clarify this: Are you voting to add an option to grdcut or do you agree with Meghan? |
Agree with Meghan. |
This addresses #6167 when we cut a subset from, say,
@earth_relief_10m
and then no longer know what the CPT should be. it is now added as a netCDF z-variable attribute and can thus be found by GMT as well as other environments. A new test (defect.sh) have been added that shows it is now working.Two older tests (polycut.sh and highpass.sh) were changed to explicitly use turbo so that they would not fail due to this improvement. Closes #6167.