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.

Sign up for GitHub

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

Jump to bottom

Wrap GMT's data structure GMT_GRID_HEADER for grid/image/cube headers #3127

Merged
merged 4 commits into from
Mar 20, 2024
Merged

Wrap GMT's data structure GMT_GRID_HEADER for grid/image/cube headers #3127

merged 4 commits into from
Mar 20, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 20, 2024

Description of proposed changes

Part of PR #2398 to support wrapping GMT_GRID/GMT_IMAGE/GMT_CUBE.

GMT_GRID_HEADRE is used in GMT_GRID/GMT_IMAGE/GMT_CUBE, so better to have it merged into the main branch then we can work on GMT_GRID/GMT_IMAGE in parallel.

Proposed by #2398 (review)

No tests are added.

@seisman seisman added the enhancement Improving an existing feature label Mar 20, 2024
@seisman seisman added this to the 0.12.0 milestone Mar 20, 2024
@seisman seisman added the needs review This PR has higher priority and needs review. label Mar 20, 2024
weiji14
weiji14 reviewed Mar 20, 2024
View reviewed changes
pygmt/datatypes/grid.py Show resolved Hide resolved
pygmt/datatypes/grid.py Outdated Show resolved Hide resolved
pygmt/datatypes/grid.py Outdated Show resolved Hide resolved
pygmt/datatypes/grid.py Outdated Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Mar 20, 2024 •
edited
Loading

Can you copy some of the nice statements about GMT_GRID_HEADER from #2398 (comment) into the first comment above? Good for future reference.

@seisman @weiji14
Apply suggestions from code review
a23e237 
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@seisman seisman mentioned this pull request Mar 20, 2024
Merged
5 tasks
weiji14
weiji14 approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

No tests are added.

Ok with this, since the tests will be added in #2398.

Pre-approving for now, but feel free or not to include the recommendations above.

@seisman
Copy link
Member Author

seisman commented Mar 20, 2024

Can you copy some of the nice statements about GMT_GRID_HEADER from #2398 (comment) into the first comment above? Good for future reference.

I'm not sure about this. Most of the statements in #2398 (comment) are about how to map GMT_GRID_HEADER metadata from/to netCDF metadata. They may make no sense to images.

@seisman seisman enabled auto-merge (squash) March 20, 2024 02:50
@seisman seisman disabled auto-merge March 20, 2024 02:55
@seisman seisman merged commit bfb641e into main Mar 20, 2024
17 of 19 checks passed
@seisman seisman deleted the datatypes/gmtgridheader branch March 20, 2024 02:55
@seisman seisman removed the needs review This PR has higher priority and needs review. label Mar 20, 2024
@weiji14
Copy link
Member

weiji14 commented Mar 20, 2024

Can you copy some of the nice statements about GMT_GRID_HEADER from #2398 (comment) into the first comment above? Good for future reference.

I'm not sure about this. Most of the statements in #2398 (comment) are about how to map GMT_GRID_HEADER metadata from/to netCDF metadata. They may make no sense to images.

I meant the GMT_GRID_HEADER data structure part, but that's ok, we can just leave it there.

@weiji14 weiji14 mentioned this pull request Mar 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiji14 weiji14 weiji14 approved these changes

Assignees
No one assigned
Labels
enhancement Improving an existing feature
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@seisman @weiji14