Deduplicate constraints and conditions#141
Conversation
g-braeunlich
force-pushed
the
90-conditions-refactor
branch
from
7002eb0 to
86539ce
Compare
ffelten
left a comment
ffelten
left a comment
There was a problem hiding this comment.
Looks way nicer 👍, good job!
My main concern: the API break with conditions_keys -- should be easy to fix.
Also, I'm wondering if we should not define conditions in another file to reduce the size of the v0 files? This would also solve the circular dependency issues when you want to type function parameters in backend with the actual config type.
| class Config(Conditions): | ||
| """Structured representation of configuration parameters for a numerical computation.""" | ||
|
|
||
| alpha: Annotated[float, bounded(lower=0.0, upper=10.0).category(THEORY)] = 0.0 |
There was a problem hiding this comment.
I think alpha being part of the config is a bit weird. Here is the info I have in mind:
- Alpha should be part of the design from an engineering perspective (angle of attack)
- The way we pass alpha to the simulator is via the templating, which takes the config and conditions dict.
- Having alpha in these dataclasses allows to check conditions, although we could include it in the designs and check conditions on the designs.
I'm not sure how to deal with this: it seems like putting alpha in these dicts makes "implementation sense" but does not make "domain-specific sense". I don't know if I'm clear at all.
There was a problem hiding this comment.
I dont mind at all.
@cashend What is your point on this?
There was a problem hiding this comment.
I think alpha being part of the config is a bit weird. Here is the info I have in mind:
Alpha should be part of the design from an engineering perspective (angle of attack)
The way we pass alpha to the simulator is via the templating, which takes the config and conditions dict.
Having alpha in these dataclasses allows to check conditions, although we could include it in the designs and check conditions on the designs.
I'm not sure how to deal with this: it seems like putting alpha in these dicts makes "implementation sense" but does not make "domain-specific sense". I don't know if I'm clear at all.
After some thought, I think this makes sense. I suggest we just keep alpha as a design variable. I also think keeping the constraint warning might be good though in case people try to use crazy AoAs.
| ("length", self.length), | ||
| ) | ||
| self.config = self.Config(**kwargs) | ||
| self.volume = self.config.volume |
There was a problem hiding this comment.
Might as well remove self.volume and replace by self.config.volume everywhere to avoid duplicates?
There was a problem hiding this comment.
@MiladHB Would that break some workflow? I.e. does any workflow access problem.volume directly?
There was a problem hiding this comment.
Hey Gerhard, Sorry for the late reply. If I understand correctly, changing self.volume to self.config.volume should not cause any problems since the simulation codes do not have access to problem.volume.
There was a problem hiding this comment.
And I guess same applies for resolution and length?
There was a problem hiding this comment.
Yes, they are similar variables.
There was a problem hiding this comment.
OK. Changed now in 2d and 3d
g-braeunlich
force-pushed
the
90-conditions-refactor
branch
2 times, most recently
from
afa937e to
a9864d6
Compare
g-braeunlich
force-pushed
the
90-conditions-refactor
branch
5 times, most recently
from
9e0c085 to
a229a66
Compare
g-braeunlich
force-pushed
the
90-conditions-refactor
branch
from
a229a66 to
f0bcc3d
Compare
|
What is the status on this? It's been hanging for while now 😅 |
|
To my knowledge, every comment should be addressed now. |
That, or a new version of mypy that has found a new potential error. I believe we can fix these ones in this PR and merge. |
g-braeunlich
force-pushed
the
90-conditions-refactor
branch
from
f0bcc3d to
cd39769
Compare
|
OK. Should be fixed now |
5 participants
No description provided.