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

Wrap solar #804

Merged
merged 136 commits into from
Mar 25, 2021
Merged

Wrap solar #804

merged 136 commits into from
Mar 25, 2021

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented Jan 23, 2021

Description of proposed changes

Wrap the GMT module solar.

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.

Notes

  • You can write /format in the first line of a comment to lint the code automatically

@willschlitzer willschlitzer added this to the 0.3.0 milestone Jan 23, 2021
@willschlitzer willschlitzer added the feature Brand new feature label Jan 23, 2021
@seisman
Copy link
Member

seisman commented Mar 23, 2021

@seisman I'm running this now; there are no changes to the .png files when I do dvc push and no changes to the .dvc files for when I add files in git. I don't know what I'm doing wrong.

Update: Tried again and there was a change?

ERROR: failed to pull data from the cloud - Checkout failed for following targets:
pygmt/tests/baseline/test_solar_terminators.png
Is your cache up to date?
<error.dvc.org/missing-files>

Did you forget to upload the images using dvc push?

@willschlitzer
Copy link
Contributor Author

I'm going to call it quits on trying to wrap solar; I'll give it another go down the line but I think this PR is costing more effort than it's worth.

@seisman
Copy link
Member

seisman commented Mar 24, 2021

@willschlitzer Sorry to see that you want to close the PR. I understand that the PR takes much longer time and effort than expected and has undergone too many rounds of revisions. However, the PR is almost ready to merge, as long as an un-related bug (#1116) detected by this PR is fixed and all tests pass.

So I'm reopening the PR.

@seisman seisman reopened this Mar 24, 2021
@weiji14
Copy link
Member

weiji14 commented Mar 24, 2021

Let's leave this PR as it is for a while and give Will some time. After some introspection, I do think we've stepped overboard and need to take a step back. People should come first, code is only secondary, so let's respect Will's decision.

@willschlitzer
Copy link
Contributor Author

willschlitzer commented Mar 24, 2021

Let's leave this PR as it is for a while and give Will some time. After some introspection, I do think we've stepped overboard and need to take a step back. People should come first, code is only secondary, so let's respect Will's decision.

@weiji14 @seisman I'm fine with it being reopened; I am just ready to work on issues that I think are more "useful" (histogram, a full tutorial, updating documentation) and feel like this PR is taking a disproportionately large amount of time and effort for the reviewers and myself. If I was too heavy-handed in closing it, my apologies, I just didn't feel like I was getting any closer to it being completed and want to move on to other issues/additions.

pygmt/tests/test_solar.py Outdated Show resolved Hide resolved
pygmt/tests/test_solar.py Outdated Show resolved Hide resolved
pygmt/tests/test_solar.py Outdated Show resolved Hide resolved
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Mar 25, 2021
@seisman
Copy link
Member

seisman commented Mar 25, 2021

I think now this PR is ready for merging.

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.

Yep, let's put this to a close and move on. I'm tempted to merge this in once the tests pass, but will wait for @willschlitzer's word on this.

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 25, 2021
@seisman seisman merged commit 5a6e4df into GenericMappingTools:master Mar 25, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
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.

4 participants