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

Plot square or cube by default for OGR/GMT files with Point/MultiPoint types #1438

Merged
merged 36 commits into from
Aug 25, 2021

Conversation

yohaimagen
Copy link
Contributor

@yohaimagen yohaimagen commented Aug 11, 2021

To keep consistency with default style plotting of geoDataFrame with Point or MultiPoint geometry see #1405 adding the same behavior for .gmt files that are saved from geoDataFrame with .to_file('some/path', driver="OGR_GMT", mode="w")

Fixes #1373

Reminders

  • Add the same if statement to plot3d.py

  • Add tests for new features or tests that would have caught the bug that you're fixing.

@yohaimagen yohaimagen self-assigned this Aug 11, 2021
@yohaimagen yohaimagen added the feature request New feature wanted label Aug 11, 2021
@yohaimagen
Copy link
Contributor Author

I am falling here on 5 tests:
when running make test I am getting
FAILED ../pygmt/tests/test_basemap.py::test_basemap_winkel_tripel
FAILED ../pygmt/tests/test_config.py::test_config_map_grid_cross_size
FAILED ../pygmt/tests/test_config.py::test_config_map_grid_pen
FAILED ../pygmt/tests/test_config.py::test_config_map_tick_length
FAILED ../pygmt/tests/test_config.py::test_config_map_tick_pen

all 5 tests do not involve plot.py which I modified and do involve basemap.py, also if I'm removing the added code to plot.py and run make test I'm getting the same test failure.
someone has a clue why do I get those results?

@yohaimagen yohaimagen added enhancement Improving an existing feature and removed feature request New feature wanted labels Aug 12, 2021
@seisman
Copy link
Member

seisman commented Aug 12, 2021

I am falling here on 5 tests:
when running make test I am getting
FAILED ../pygmt/tests/test_basemap.py::test_basemap_winkel_tripel
FAILED ../pygmt/tests/test_config.py::test_config_map_grid_cross_size
FAILED ../pygmt/tests/test_config.py::test_config_map_grid_pen
FAILED ../pygmt/tests/test_config.py::test_config_map_tick_length
FAILED ../pygmt/tests/test_config.py::test_config_map_tick_pen

all 5 tests do not involve plot.py which I modified and do involve basemap.py, also if I'm removing the added code to plot.py and run make test I'm getting the same test failure.
someone has a clue why do I get those results?

Maybe try to run dvc pull again to see if these images are up-to-date?

@yohaimagen
Copy link
Contributor Author

yohaimagen commented Aug 17, 2021

Running dvc pull does not solve the issue. Also if I am making a fresh new development environment as described and run make test I am getting the same failed tests(on the main branch).
ping @weiji14 maybe you know how to solve this issue?

@weiji14
Copy link
Member

weiji14 commented Aug 17, 2021

Running dvc pull does not solve the issue. Also if I am making a fresh new development environment as described and run make test I am getting the same failed tests(on the main branch).
ping @weiji14 maybe you know how to solve this issue?

There is only 1 test failing on CI (test_legend_entries at https://github.com/GenericMappingTools/pygmt/pull/1438/checks?check_run_id=3302244951#step:11:735) instead of 5. The error message is as follows:

        if "S" in kwargs and kwargs["S"][0] in "vV" and direction is not None:
            extra_arrays.extend(direction)
        if "S" not in kwargs and kind == "file":
>           with open(data, "r") as file:
E           FileNotFoundError: [Errno 2] No such file or directory: '@Table_5_11.txt'

The problem is that your code tries to open the @Table_5_11.txt file, but because this is a remote GMT file from the server, Python can't find it. The easiest way to pass that test would be to add another if condition (if data.endswith(".gmt")) so that only OGR/GMT files with the .gmt extension are checked.

However, users may want to plot OGR/GMT files from https://github.com/GenericMappingTools/gmtserver-admin/tree/master/cache, so we'll need to handle files like @quakes.gmt too... Can you try to come up with another solution for that case?

@yohaimagen
Copy link
Contributor Author

We can use os.path.exists to check if the file is on the system.

  1. I don't know what is the import police can I import os at plot.py?
  2. That means that we do not support remote GMT files from the server and doing something like
fig = pygmt.Figure()
fig.basemap(projection="M5c", frame="a", region=[0, 30, 0, 30])
fig.plot(data="@quakes.gmt")
fig.show()

will plot a line instead of squares.

as I understand from the code the import of files from the server is done on the GMT side so we can't support it anyway.

@yohaimagen
Copy link
Contributor Author

Another option that we can use is to do something like that:

try:
    with open(data, 'r') as file:
        line = file.readline()
        if "@GMULTIPOINT" in line or "@GPOINT" in line:
            kwargs["S"] = "s0.2c"
except FileNotFoundError:
        pass

We do not import os in that solution, but we still do not support remote files, as I mentioned that's up to GMT.

@weiji14
Copy link
Member

weiji14 commented Aug 18, 2021

I like the try-except style, could you make the change please?

Another thing we could do (if we want to support remote OGR/GMT files) is to use pygmt.which to locate the local path of the @something.gmt file, and have the open function read that. Oh and by the way, it's ok to put import os in the plot.py file since it's a standard Python module.

@seisman
Copy link
Member

seisman commented Aug 18, 2021

Another thing we could do (if we want to support remote OGR/GMT files) is to use pygmt.which to locate the local path of the @something.gmt file, and have the open function read that.

I think this way is better.

It's not only about supporting remote files. GMT users can store their data (e.g., faults.gmt, plate_boundary.gmt) in any directory and set the environment variable GMT_DATADIR to that directory, GMT will also look for files in that directory. So users can use

fig.plot("faults.gmt")

even though the file faults.gmt is not in the current directory and is not a remote file.

@yohaimagen
Copy link
Contributor Author

I changed the code to use pygmt.which and kept the try-except phrase so we will have consistency on the error thrown in case that really the file does not exists

pygmt/src/plot.py Outdated Show resolved Hide resolved
pygmt/src/plot.py Outdated Show resolved Hide resolved
pygmt/src/plot3d.py Outdated Show resolved Hide resolved
pygmt/src/plot3d.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Aug 18, 2021

@yohaimagen Need to resolve the conflicts first.

@seisman seisman added this to the 0.5.0 milestone Aug 18, 2021
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
yohaimagen and others added 2 commits August 19, 2021 09:12
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@yohaimagen
Copy link
Contributor Author

O.K I added the encoding to the open commands in the tests and the # pylint: disable=too-many-locals to the functions.

And we passing the style check :)

Let me know if there is something else to do over here.

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.

Just some more typos, otherwise good for a final round of reviews! We'll merge in 24 hours if there are no other issues 😁

pygmt/tests/test_plot.py Outdated Show resolved Hide resolved
pygmt/tests/test_plot3d.py Outdated Show resolved Hide resolved
pygmt/tests/test_plot3d.py Outdated Show resolved Hide resolved
@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Aug 24, 2021
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
yohaimagen and others added 2 commits August 24, 2021 14:42
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@yohaimagen
Copy link
Contributor Author

We have one error: the docs were unable to build on windows, where it falls on something unrelated to this pull request.
It is falling while trying to read the iris data at scatterplot3d.py

df = pd.read_csv("https://github.com/mwaskom/seaborn-data/raw/master/iris.csv")

Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks good, except a few minor edits

pygmt/tests/test_plot.py Outdated Show resolved Hide resolved
pygmt/tests/test_plot3d.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Aug 24, 2021

We have one error: the docs were unable to build on windows, where it falls on something unrelated to this pull request.
It is falling while trying to read the iris data at scatterplot3d.py

df = pd.read_csv("https://github.com/mwaskom/seaborn-data/raw/master/iris.csv")

This error can be ignored, because we already know that sometimes GMT API on Windows may crash.

Copy link
Member

@michaelgrund michaelgrund left a comment

Choose a reason for hiding this comment

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

Except the minor suggestions this PR looks good to me!

pygmt/tests/test_plot.py Outdated Show resolved Hide resolved
pygmt/tests/test_plot3d.py Outdated Show resolved Hide resolved
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
yohaimagen and others added 4 commits August 25, 2021 07:27
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Michael Grund <23025878+michaelgrund@users.noreply.github.com>
Co-authored-by: Michael Grund <23025878+michaelgrund@users.noreply.github.com>
@weiji14 weiji14 changed the title Plot square or cube by default for Point/MultiPoint gmt file types Plot square or cube by default for Point/MultiPoint type gmt files Aug 25, 2021
@weiji14 weiji14 changed the title Plot square or cube by default for Point/MultiPoint type gmt files Plot square or cube by default for OGR/GMT files with Point/MultiPoint types Aug 25, 2021
@weiji14 weiji14 merged commit 0881d0f into main Aug 25, 2021
@weiji14 weiji14 deleted the gmt-shape-file-point-default-style branch August 25, 2021 10:59
@weiji14 weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Aug 25, 2021
@weiji14
Copy link
Member

weiji14 commented Aug 25, 2021

Appreciate your help @yohaimagen, thanks again! 🥳

@yohaimagen
Copy link
Contributor Author

Thanks to all three reviewers for reviewing my code.

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…t types (GenericMappingTools#1438)

Set default behavior for plotting OGR/GMT files with
Point/Multipoint type geometry as points (-Sp0.2c) for `plot`
and cubes (-Su0.2c) for `plot3d`, instead of GMT's default
line style plot.

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: Michael Grund <23025878+michaelgrund@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot point type geopandas.GeoDataFrames as points directly instead of as lines
4 participants