-
Notifications
You must be signed in to change notification settings - Fork 99
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
Time spectral with flexible grid #169
Conversation
Codecov Report
@@ Coverage Diff @@
## master #169 +/- ##
===========================================
- Coverage 42.74% 27.66% -15.08%
===========================================
Files 15 15
Lines 3495 3502 +7
===========================================
- Hits 1494 969 -525
- Misses 2001 2533 +532
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will review the code soon, but I wanted to comment on the options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks fine, but I would like some clarification on the following points
end subroutine gridVelocitiesFineLevel_TS | ||
#endif | ||
|
||
subroutine gridVelocitiesFineLevel_TS_block(nn, sps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be AD code for this like there is for gridVelocitiesFineLevel_block
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It is in my branch and will be pushed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to make the same comment. I don't see the AD'd version of this subroutine in this PR.
Could we merge now? Do I need to do anything else? @sseraj @marcomangano |
I haven't reviewed this yet; I wasn't sure if it was ready or not. Can you give me a few days to review this? |
@anilyil Sounds good. Thanks! |
I assume the test will keep failing until we fix the issue with Azure, so we can ignore it for now. I don't have additional recommendations, but I will let the experts review this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for the delay. I left a few comments. Thanks!
end subroutine gridVelocitiesFineLevel_TS | ||
#endif | ||
|
||
subroutine gridVelocitiesFineLevel_TS_block(nn, sps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to make the same comment. I don't see the AD'd version of this subroutine in this PR.
@SichengHe please take a look at the reviewer comments |
I think the only thing still left to do here is to add the AD code? |
@sseraj Could we actually merge this first? I am planning to create another PR for the ADed code later this month. |
adding the AD code later is fine for me. @sseraj up to you if you want the AD code in this PR. Otherwise this is good to go for me |
@sseraj @SichengHe what is the decision then? Shall we wait for the ADed code, or shall we move on and merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have the AD code in the same PR but I am fine with merging this to avoid the PR getting stale.
Purpose
The commit is related to time spectral with a flexible grid. It also contains a simple 2D regression test case.
Type of change
Testing
Checklist
flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted