Fixed multiple BC DV and setRotationRate rate for local blocks #323
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shubham135sd
changed the title
Codecov Report
@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 41.94% 41.96% +0.01%
==========================================
Files 13 13
Lines 4005 4006 +1
==========================================
+ Hits 1680 1681 +1
Misses 2325 2325
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
lamkina
requested changes
Sep 29, 2023
There was a problem hiding this comment.
A few preliminary things:
- Please run black on the python layer and fprettify on the fortran layer.
- For the BC fixes, can you add tests that demonstrate your changes fix the bug? Testing on a local case won't be enough here because you are changing manually differentated routines.
- Can you add a test for the rotation rate fix? I would like to see a test that demonstrates the expected capability and shows your changes achieve that capability.
| @@ -6309,6 +6310,7 @@ def _getObjectivesAndDVs(self): | |||
| "mavgptot": self.adflow.constants.costfuncmavgptot, | |||
| "aavgptot": self.adflow.constants.costfuncaavgptot, | |||
| "aavgps": self.adflow.constants.costfuncaavgps, | |||
| "mavgrho": self.adflow.constants.costfuncmavgrho, | |||
There was a problem hiding this comment.
What is this cost function that you're adding?
|
I would prefer two separate PRs. |
|
Closing this PR due to inactivity. I will shortly make a PR to address the multi BC issue and link it to this PR. There is an issue open (#353 ) that addresses the other aspect of the PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Currently when multiple BC DVs of the same type are applied (e.g. bcinflowsubsonic, bcoutflowsubsonic), the second instance of the BC and its derivatives are overwritten on to the first BC and the values of the second BC remain unchanged and its derivatives are zero. The current PR fixes both the BC value change and its derivative update, which are manually differentiated routines.
Fixes
setRotationRate(rotCenter, rotRate, cgnsBlocks=None). Currently when cgnsBlocks are passed, adflow adds rotation rate to 'n' cgnsBlocks starting from the first block number instead of applying the rotation rate to the specified block ids. The changes in this PR fix this bug.Expected time until merged
One week
Type of change
Testing
Proper test case to be created.
Checklist