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

WIP: Seperates families and familyGroups #34

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
Draft

Conversation

joanibal
Copy link
Collaborator

This is a draft PR because I want to get feedback on the idea before adding the PRs for the other repos and totally polishing the code.

Purpose

Until now families and family groups have been used interchangeably by aerosolvers to refer to the list of surface families defined in the cgns mesh and those added by the user.

This is a problem if you want to iterate over the families on an aerosolver because the families defined by the user may contain surfaces already in another family.
It is because of this I propose separating the list of families and family groups in the AeroSolver class.
The families dictionary will contain only the original families defined in the mesh file.
The family groups dictionary will contain all the original families, since each is a collection of a single family (itself), and all the additional family groups defined by the aerosolver and user.

for example:

        self.families              | 'inlet'  'outlet' 'nozzle_wall'  |
        (from surface definition)  |   [1]       [2]       [3]        |
                                    ----------------------------------

                           -----------------------------------------------------------------
        self.familyGroups | 'inlet'  'outlet' 'nozzle_wall' 'allwalls' 'allsurfaces'        |
        (collection of    |   [1]       [2]       [3]          [3]       [1, 2, 3]          |
         families)         -----------------------------------------------------------------

This will make if feasible to do things like iterate over the families defined in the cgns mesh to return boundary condition information.

Type of change

  • New feature (non-breaking change which adds functionality)

Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

I went through the changes but I need some more time to digest it... and understand our current code.
I see the point of this PR and makes sense to me, but I have some doubts (see my questions).
I feel we need a bit more documentation about the whole baseclasses magic, but we proabably want to deal with it in a separate PR.

except KeyError:
raise Error("The value must be given or set using the setActuatorVar routine")

# # the value of the BCData[familyGroup][key] maybe a dictionary of data for each patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed? I got the point codewise, but I am wondering if we need or want to allow this kind of nested dictionaries in the BCdata definition

def getBCData(self):
return self.bc_data

# def setBCArray(self, groupName, varName, dataArray, patch=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? I guess this is related to the "nested" approach below, I am wondering for which cases having an array as an input would make sense

def getActuatorData(self):
return self.actuatorData

# def addBCDV(self, bc_var, value=None, lower=None, upper=None, scale=1.0,
Copy link
Contributor

@marcomangano marcomangano Jan 19, 2021

Choose a reason for hiding this comment

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

When could this be useful?

else:
self.bcVarData[key, family] = value
setattr(self, key, value)
Copy link
Contributor

@marcomangano marcomangano Jan 19, 2021

Choose a reason for hiding this comment

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

just a clarification on the "original" code: what vars would we be adding here? I assume there are previous checks on the key to be sure we are not adding a random name/var

self.size = value.size
else:
size = None
# TODO raise an error?
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense... unless we want to "filter" these inputs before we instantiate the aeroDV container.
Are we using this size attribute anywhere yet? Definitely useful anyway

return aeroDvsDot, bcDvsDot, actDvsDot

def evalDVsSensBwd(self, aeroDvBar, BCDataBar, actDataBar):
"""This internal furncion is used to convert the raw array output from
Copy link
Contributor

Choose a reason for hiding this comment

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

Two dumb questions here:

  • Why the fwd and bwd function differ (i.e. why we have a different treatment for mach and altitude only for the bwd sens)?
  • how are these functions going to be used, and how are we currently dealing with the sens raw data?

@marcomangano
Copy link
Contributor

@joanibal what is the status of this PR? What bits are missing?

@eirikurj
Copy link
Contributor

@joanibal what is the state of this PR?

@joanibal
Copy link
Collaborator Author

It has been moved way to the back of my TODOs.

Is there I way I can mark it inactive?

@ewu63 ewu63 changed the title Seperates families and familyGroups WIP: Seperates families and familyGroups Mar 19, 2021
@ewu63
Copy link
Collaborator

ewu63 commented Mar 19, 2021

I added an app today actually, which is a status check that will be set to "in progress" if the PR title contains "WIP". I can set it to required which will prevent this from being merged. But in any case the draft PR should be fine, I think they just wanted to double check the status here.

@eirikurj
Copy link
Contributor

@joanibal, any updates on this PR? Do you have plans to work on this?

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.

4 participants