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

Feature/reimplement nearneighbour #65

Merged
merged 8 commits into from
Jul 12, 2021

Conversation

JamieJQuinn
Copy link
Collaborator

This PR reimplements the nearneighbour call using pyGMT's clib instead of a manual os call to gmt. This is intended to help with the performance impact of writing temporary files (see #60). Although GMT will still write to a temporary file, pyGMT uses virtual files to ensure we don't have to write the input to GMT, so some savings should come from that. Additionally, we don't have to handle temporary files ourselves.

@JamieJQuinn
Copy link
Collaborator Author

I'm so confused why only macos and ubuntu 3.6 are failing...

Copy link
Collaborator

@alessandrofelder alessandrofelder 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 to me apart from some versioning issues for pyGMT and Python 3.6.
Seems that the ubuntu and mac CI install pygmt-0.2.1 which lacks some of the Clib functionality that we want to use?
Maybe worth trying to specify the minimal pygmt version required?

@alessandrofelder
Copy link
Collaborator

(Also at a later timepoint/if we have time, it might be cool to move this implementation into pygmt itself)

Copy link
Collaborator

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Installing pygmt>=0.3.1 seems to have done the trick.
@JamieJQuinn can you update this in the README instructions on how to create an appropriate conda environment too, please, and then merge?
@Devaraj-G if you've got a pygmt version <0.3.1 in your cascadia conda environment (check this with conda list and then scroll down to p), the easiest solution is to remove your conda environment (conda env remove -n cascadia) and then re-create it following the instructions in the README, after this is merged (if you're using Python 3.9 you can ignore this).

@Devaraj-G
Copy link
Collaborator

@alessandro pygmt version is 0.3.1. Thanks for the heads-up. Proper to have it in the README.

@JamieJQuinn
Copy link
Collaborator Author

Perhaps could have been its own PR but I've added environment.yml which is the equiv of requirements.txt for conda. Should simplify both the install instructions (which have been updated) and the CI (which has been updated).

@JamieJQuinn JamieJQuinn dismissed alessandrofelder’s stale review July 8, 2021 15:58

Significant changes have been made since last review

@JamieJQuinn
Copy link
Collaborator Author

Phew! That could have gone better... @alessandrofelder take a look and reapprove if you're happy.

Copy link
Collaborator

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Awesome! Love how nicely this simplifies the setup instructions.

@JamieJQuinn JamieJQuinn merged commit 7aa8015 into main Jul 12, 2021
@JamieJQuinn JamieJQuinn deleted the feature/reimplement-nearneighbour branch July 12, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants