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

Add new function to load fractures sample data #1101

Merged
merged 21 commits into from Mar 24, 2021
Merged

Conversation

michaelgrund
Copy link
Member

@michaelgrund michaelgrund commented Mar 22, 2021

Description of proposed changes

As suggested in #794 here's the function implementation to load the fractures_06.txt sample data.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

As suggested in #794 here's the function implementation to load the fractures_06.txt sample data.
@michaelgrund michaelgrund added the maintenance Boring but important stuff for the core devs label Mar 22, 2021
@maxrjones
Copy link
Member

Are you sure that fracture_06.txt includes lengths? It seems too coincidental that column 0 increments by one (viewable here: https://github.com/GenericMappingTools/gmtserver-admin/blob/master/cache/fractures_06.txt)

@maxrjones
Copy link
Member

Are you sure that fracture_06.txt includes lengths? It seems too coincidental that column 0 increments by one (viewable here: https://github.com/GenericMappingTools/gmtserver-admin/blob/master/cache/fractures_06.txt)

I noticed that this is copied from GMT's example 6. @PaulWessel, do the lengths in @fracture_06.txt represent real fractures or should we specify that the lengths are synthetic and the directions are digitized from maps?

@michaelgrund
Copy link
Member Author

michaelgrund commented Mar 22, 2021

Are you sure that fracture_06.txt includes lengths? It seems too coincidental that column 0 increments by one (viewable here: https://github.com/GenericMappingTools/gmtserver-admin/blob/master/cache/fractures_06.txt)

If I remember correctly the lengths are located in column 1 and directions in column 0 (therefore 0 to 360°). That's also the reason why to use columns=[1, 0] in the rose gallery example. Otherwise I cannot reproduce the figure from the GMT docs (https://docs.generic-mapping-tools.org/latest/gallery/ex06.html). Maybe @PaulWessel can shed some light on this.

@maxrjones
Copy link
Member

Are you sure that fracture_06.txt includes lengths? It seems too coincidental that column 0 increments by one (viewable here: https://github.com/GenericMappingTools/gmtserver-admin/blob/master/cache/fractures_06.txt)

If I remember correctly the lengths are located in column 1 and directions in column 0 (therefore 0 to 360°).

Are you sure that fracture_06.txt includes lengths? It seems too coincidental that column 0 increments by one (viewable here: https://github.com/GenericMappingTools/gmtserver-admin/blob/master/cache/fractures_06.txt)

If I remember correctly the lengths are located in column 1 and directions in column 0 (therefore 0 to 360°). That's also the reason why to use columns=[1, 0] in the rose gallery example. Otherwise I cannot reproduce the figure from the GMT docs (https://docs.generic-mapping-tools.org/latest/gallery/ex06.html). Maybe @PaulWessel can shed some light on this.

Yes, the order makes sense since the gmt rose example uses -: to switch the column order. The directions being evenly incremented still seems a bit odd to me unless the 'compilation' refers to a binned data set.

@seisman seisman mentioned this pull request Mar 22, 2021
5 tasks
@PaulWessel
Copy link
Member

The data set is fake I think but I cannot recall for sure. This takes us back to 1990. While some guys give roses to their love-interests, I wrote psrose for my future wife to impress her (she needed to make these plots for her thesis). Even nerds know that women likes flowers.

@michaelgrund
Copy link
Member Author

The data set is fake I think but I cannot recall for sure. This takes us back to 1990. While some guys give roses to their love-interests, I wrote psrose for my future wife to impress her (she needed to make these plots for her thesis). Even nerds know that women likes flowers.

Funny to hear some things from the very beginning of psrose @PaulWessel, especially since the corresponding example was the first I was faced with when I started using GMT in 2011 😄.

@maxrjones
Copy link
Member

The data set is fake I think but I cannot recall for sure. This takes us back to 1990. While some guys give roses to their love-interests, I wrote psrose for my future wife to impress her (she needed to make these plots for her thesis). Even nerds know that women likes flowers.

I'm glad to have asked the question to learn this story about the origins of the psrose module. My spouse and I have given up on flowers due to allergies - perhaps next anniversary we should just use GMT instead 😄

@michaelgrund - up to you about whether it needs to be specified that the data may be made up. You could add something like 'hypothetically digitized'

Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

Can you add a test to 'pygmt/tests/test_datasets_tutorial.py`?

@michaelgrund
Copy link
Member Author

The data set is fake I think but I cannot recall for sure. This takes us back to 1990. While some guys give roses to their love-interests, I wrote psrose for my future wife to impress her (she needed to make these plots for her thesis). Even nerds know that women likes flowers.

I'm glad to have asked the question to learn this story about the origins of the psrose module. My spouse and I have given up on flowers due to allergies - perhaps next anniversary we should just use GMT instead 😄

@michaelgrund - up to you about whether it needs to be specified that the data may be made up. You could add something like 'hypothetically digitized'

Adjusted 😉.

@michaelgrund
Copy link
Member Author

Can you add a test to 'pygmt/tests/test_datasets_tutorial.py`?

Done! Thanks for mentioning @meghanrjones.

@seisman
Copy link
Member

seisman commented Mar 22, 2021

If I remember correctly the lengths are located in column 1 and directions in column 0 (therefore 0 to 360°). That's also the reason why to use columns=[1, 0] in the rose gallery example. Otherwise I cannot reproduce the figure from the GMT docs (docs.generic-mapping-tools.org/latest/gallery/ex06.html).

If I understand it correctly, rose expects length and azimuth, but the data gives azimuth and length. Do you think we should swap the column order returned by the function?

@PaulWessel
Copy link
Member

From man page: rose reads (length, azimuth) pairs from file [or standard input] and plot a windrose diagram. Add -i0 if your file only has azimuth values

This is a bit unfortunate as it would make lots more sense that azimuth would be the first column since azimuths are always required and lengths are optional. I am not sure if there is a way to fix this in a backwards compatible way though.

@michaelgrund
Copy link
Member Author

If I remember correctly the lengths are located in column 1 and directions in column 0 (therefore 0 to 360°). That's also the reason why to use columns=[1, 0] in the rose gallery example. Otherwise I cannot reproduce the figure from the GMT docs (docs.generic-mapping-tools.org/latest/gallery/ex06.html).

If I understand it correctly, rose expects length and azimuth, but the data gives azimuth and length. Do you think we should swap the column order returned by the function?

Would be fine for me, however, for advanced GMT users which are familiar with this "old" example there may be some confusion.

This is a bit unfortunate as it would make lots more sense that azimuth would be the first column since azimuths are always required and lengths are optional. I am not sure if there is a way to fix this in a backwards compatible way though.

It would be easy to fix that for PyGMT only, however, I would prefer to ensure consistency with GMT. Any comments on that?

@PaulWessel
Copy link
Member

At least for GMT core we need to maintain backwards compatibility. Here are the options we have I think:

  1. Add a new option to psrose that explicitly selects azimuth or azimuth, length input. Unfortunately, most letters are already used, leaving perhaps only -H. With this solution, a missing -H would mean the old format, hence backwards compatible. It would be listed as a required option, except we would not enforce the requirement so that backwardness would work.
  2. Add a new module pscircular that uses the input order we wish to have. pscircular is just a few lines of C code setting up the input and then calling the module psrose via GMT_Call_Module. psrose would remain as a module but no longer documented, hence not breaking anyone's old scripts.

I am not sure if the problem is severe enought to consider those options though. With pscircular we get a fresh page and can redesign those messy options into something better since backwards compatibility is no longer an issue (as long as we can construct psrose commands from them)..

@maxrjones
Copy link
Member

At least for GMT core we need to maintain backwards compatibility. Here are the options we have I think:

1. Add a new option  to **psrose** that explicitly selects azimuth or azimuth, length input.  Unfortunately, most letters are already used, leaving perhaps only **-H**. With this solution, a missing -H would mean the old format, hence backwards compatible.  It would be listed as a required option, except we would not enforce the requirement so that backwardness would work.

2. Add a new module **pscircular** that uses the input order we wish to have.  **pscircular** is just a few lines of C code setting up the input and then calling the module **psrose** via _GMT_Call_Module_.  **psrose** would remain as a module but no longer documented, hence not breaking anyone's old scripts.

I am not sure if the problem is severe enought to consider those options though. With pscircular we get a fresh page and can redesign those messy options into something better since backwards compatibility is no longer an issue (as long as we can construct psrose commands from them)..

I don't think any core gmt changes are needed here. I think @seisman was just suggesting that the PyGMT function to load in the fractures data could switch the columns, so that -: isn't needed.

@seisman
Copy link
Member

seisman commented Mar 22, 2021

At least for GMT core we need to maintain backwards compatibility. Here are the options we have I think:

1. Add a new option  to **psrose** that explicitly selects azimuth or azimuth, length input.  Unfortunately, most letters are already used, leaving perhaps only **-H**. With this solution, a missing -H would mean the old format, hence backwards compatible.  It would be listed as a required option, except we would not enforce the requirement so that backwardness would work.

2. Add a new module **pscircular** that uses the input order we wish to have.  **pscircular** is just a few lines of C code setting up the input and then calling the module **psrose** via _GMT_Call_Module_.  **psrose** would remain as a module but no longer documented, hence not breaking anyone's old scripts.

I am not sure if the problem is severe enought to consider those options though. With pscircular we get a fresh page and can redesign those messy options into something better since backwards compatibility is no longer an issue (as long as we can construct psrose commands from them)..

I don't think any core gmt changes are needed here. I think @seisman was just suggesting that the PyGMT function to load in the fractures data could switch the columns, so that -: isn't needed.

Yes, I agree. I think the recent suggestions (#1101 (comment)) made by @meghanrjones can help remove -: or -i1,0.

@weiji14
Copy link
Member

weiji14 commented Mar 23, 2021

Yes, probably better to rename things in a separate PR.

michaelgrund added a commit that referenced this pull request Mar 23, 2021
As discussed in #1101 here's a PR to rename ``tutorial.py`` (in which functions are defined to load sample data from the GMT tutorials) to ``samples.py``.
@michaelgrund michaelgrund added this to the 0.4.0 milestone Mar 23, 2021
@michaelgrund
Copy link
Member Author

Yes, probably better to rename things in a separate PR.

Opened #1110 for this.

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.

This looks good enough to me, thanks @michaelgrund!

@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Mar 24, 2021
pygmt/datasets/samples.py Outdated Show resolved Hide resolved
pygmt/datasets/samples.py Outdated Show resolved Hide resolved
pygmt/datasets/samples.py Outdated Show resolved Hide resolved
michaelgrund and others added 4 commits March 24, 2021 17:30
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@michaelgrund michaelgrund merged commit 9ecb7d8 into master Mar 24, 2021
@michaelgrund michaelgrund deleted the load-fractures-data branch March 24, 2021 17:29
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 24, 2021
@weiji14 weiji14 added feature Brand new feature and removed maintenance Boring but important stuff for the core devs labels Jun 20, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
)

Co-authored-by: Meghan Jones <meghanj@hawaii.edu>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants