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

Fixed multiple BC DV and setRotationRate rate for local blocks #323

Closed
wants to merge 3 commits into from

Conversation

shubham135sd
Copy link

@shubham135sd shubham135sd commented Sep 29, 2023

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

  • Bugfix (non-breaking change which fixes an issue)

Testing

Proper test case to be created.

Checklist

  • I have run unit and regression tests which pass locally with my changes
  • I have tested locally that my fix is effective or that my feature works

@shubham135sd shubham135sd changed the title Fixed rotation rate for local blocks Fixed multiple BC DV and setRotationRate rate for local blocks Sep 29, 2023
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Merging #323 (bee9bd1) into main (559a1de) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            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              
Files Coverage Δ
adflow/pyADflow.py 68.95% <100.00%> (+0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

A few preliminary things:

  1. Please run black on the python layer and fprettify on the fortran layer.
  2. 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.
  3. 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this cost function that you're adding?

@lamkina
Copy link
Contributor

lamkina commented Sep 29, 2023

Also, this PR is adressing two separate bugs and I wonder if it should be split into two PR's? @eirikurj @sabet @anilyil do you have thoughts on this?

@eirikurj
Copy link
Contributor

eirikurj commented Oct 5, 2023

I would prefer two separate PRs.

@lamkina
Copy link
Contributor

lamkina commented May 14, 2024

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.

@lamkina lamkina closed this May 14, 2024
@lamkina lamkina mentioned this pull request May 14, 2024
13 tasks
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.

None yet

3 participants