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.

Sign up for GitHub

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

Jump to bottom

bug fix. remove cycle #1330

Merged
merged 2 commits into from
Jan 31, 2022
Merged

bug fix. remove cycle #1330

merged 2 commits into from
Jan 31, 2022

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Jan 31, 2022 •
edited
Loading

remove buggy cycle

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner January 31, 2022 14:56
@weiyuan-jiang weiyuan-jiang added 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. bugfix labels Jan 31, 2022
@weiyuan-jiang
Copy link
Contributor Author

I would like to replace #1329 with this PR if indeed this PR fix the issue #1328

@tclune tclune self-requested a review January 31, 2022 14:59
tclune
tclune reviewed Jan 31, 2022
View reviewed changes
tclune
tclune previously requested changes Jan 31, 2022
View reviewed changes
Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

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

See inline comment.

@mathomp4
Copy link
Member

I've put a blocker on my #1329 PR for now because this PR is here.

My question is: did you run GEOSgcm with this fix? I'd rather not merge it in if you haven't (the checkbox is not checked).

@tclune
Copy link
Collaborator

tclune commented Jan 31, 2022

I should also add that I don't want @mathomp4 to second-guess himself on such things. Unless the "fix" is completely obvious, the right process is to revert (if possible) and then await an improved PR. Should not be much more work, but correctly places the burden on the person that did the original PR.

@weiyuan-jiang
Copy link
Contributor Author

I am running it.....

@weiyuan-jiang
Copy link
Contributor Author

Yes, I have tested GEOSgcm and it runs

@mathomp4 mathomp4 requested review from tclune and mathomp4 January 31, 2022 20:52
@mathomp4 mathomp4 dismissed tclune’s stale review January 31, 2022 21:09

This has been taken care of.

@mathomp4 mathomp4 merged commit ccccd31 into develop Jan 31, 2022
@mathomp4 mathomp4 deleted the bugfix/wjiang/remove_cycle branch January 31, 2022 21:09
@mathomp4 mathomp4 mentioned this pull request Feb 1, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathomp4 mathomp4 mathomp4 approved these changes

@tclune tclune Awaiting requested review from tclune

Assignees
No one assigned
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@weiyuan-jiang @mathomp4 @tclune