Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create geosoft_gxf_io.py #539
base: main
Are you sure you want to change the base?
Create geosoft_gxf_io.py #539
Changes from all commits
43880be
5b41391
cea9cb3
42c2d0e
3dcf98d
d8e3fa0
aa0c6ce
ca785fd
a7081c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 suspect that these paragraphs are coming from this PDF: https://pubs.usgs.gov/of/1999/of99-514/grids/gxf.pdf. Am I right?
Do we have permission to reproduce the same text here? Can we double check the license of those files? In doubt, I would remove these lines from this file, a reference should be enough. If we do have permission we can leave them, but I would make sure we are compliant with the license (reference the document, acknowledgement, include license text, 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 am happy to remove 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.
From what I understood these lines are populating the
data_list
with the values of the grid. Since the number of lines for the grid are usually going to be way larger than the header lines, why not usingnp.loadtxt
to read them? It performs much faster than appending lines to a list, it already returns an array in the right shape, and it also has some sanity checks (the number of elements per row are consistent) that we don't have to take care of.I think we could start reading the header lines until we find the
#GRID
line, keep record of the number of rows for the header and stop the iteration. Then we can usenp.loadtxt
to read the rest of the file by passing the number of roads toskiprows
(+/- 1, we need to figure out the correct indexing).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.
In lines https://github.com/fatiando/harmonica/pull/539/files#diff-5ac31ec9d8d11b47ff39f59bf25e1cad43484857580dd7b6c65cddd8e0f504cfR219-R229 we populate the
metadata
dictionary with parsed values (float, int, stripped str). Isn't it better to usemetadata
to get the number of rows and columns, instead of having to parse them again as ints?That being said, maybe we want the keys in
metadata
to be lowercase (as you do in following lines withnx
,ny
,x_inc
, 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 think this function is nice for debugging the reader. But I hardly see users using it (we would need to expose it as well). Instead, if
read_gxf
returns a DataArray, then we don't need this function:xarray
can nicely display all the data.What do you think? Is there any special reason to include this function?