search

jaheyns / CfdOF

Computational Fluid Dynamics (CFD) for FreeCAD based on OpenFOAM solver
GNU Lesser General Public License v3.0
442 stars 84 forks source link

Feature/function obj interface - [merged] #119

Closed oliveroxtoby closed 1 year ago

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 10, 2022, 09:14

_Merges feature/function_objinterface -> develop

First attempt at a function objects module, forces and forceCoeffs currently available, forces (pressure + viscous) plotted to XY-plot

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 10, 2022, 09:14

requested review from @oliveroxtoby

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 10, 2022, 09:18

added 1 commit

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 10, 2022, 09:19

added 14 commits

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 10, 2022, 09:22

marked this merge request as ready

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 11, 2022, 07:16

added 13 commits

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 12, 2022, 05:44

added 1 commit

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 12, 2022, 09:00

added 10 commits

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 12, 2022, 09:08

added 1 commit

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:01

added 1 commit

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/functionobjects/CfdFunctionObjects.py line 86

This shouldn't be needed, unless we want some colour coding. Probably don't need to worry for now.

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Looking great, thanks a lot for this.

One general request: I don't think we should try to to handle all function objects with one interface. There are so many different ones which play all kinds of different roles in OpenFOAM, I think it might be nightmare to try to handle them all together in the same way.

In terms of logical grouping, what comes to mind first are things like forces, forceCoeffs, min/max/mean of field values, and probes. One could perhaps categorise these all as 'Reporting functions' or something similar.

So in summary, my request would be to rename this from an interface for function objects, to an interface for reporting functions. I have also pushed a proposed icon that I think could work if you agree.

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/functionobjects/CfdFunctionObjects.py line 98

I would not expose the field names to the user, as it's predetermined how it should be set up for each analysis type.

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/functionobjects/CfdFunctionObjects.py line 108

It might be nice to automatically set this according to presence/absence of a porous zone in the analysis.

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/functionobjects/CfdFunctionObjects.py line 120

I would go for App::PropertyPoint for this for brevity (see e.g. ExtrusionAxisPoint in CfdMeshRefinement).

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/functionobjects/CfdFunctionObjects.py line 134

I would use App::PropertyVector for the directions

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/gui/TaskPanelCfdFunctionObjects.ui line 1

Although I originally asked for indents of one space as you have here (to be the same as my qtdesigner), I think we concluded we should standardise on four spaces? Would that make sense?

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/plotters/CfdForceCoeffPlot.py line 45

residuals_forces -> forces

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on core/plotters/CfdForcesPlot.py line 1

I think it should be possible to use one class for both forces and forceCoeffs by passing in a few parameters. Might be nice to avoid too much code duplication.

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on data/defaults/system/controlDict line 111

Uncomment?

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 12, 2022, 15:02

Commented on CfdRunnableFoam.py line 200

Rename ...Residuals -> ...Forces

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 04:18

Commented on core/plotters/CfdForcesPlot.py line 1

hi @oliveroxtoby yes, of course. As indicated, I was pushing some code as a first impression. There is obviously a lot of refactoring that can happen. I've have just tried to get working POC's into the repo as a starting point. I fact, I think a single parameterised class for all the plotting we have at the moment will suffice.

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 04:20

Commented on data/defaults/system/controlDict line 111

Not yet, there is some review on how we want to / how best / a standardised way to input vector data. The one input per component is tiresome to say the least. Again, I wasnt sure if we would even want this inclusion, in keeping with the whole "keeping it simple / minimal fiddle factors etc" ethos. LMK :grin:

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 04:21

Commented on core/functionobjects/CfdFunctionObjects.py line 120

Thanks, this is what I was looking for, unfortunately there is almost NIL on the internal API. Even the Doxygen stuff

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 04:28

Yes, that was the intention, hence only Forces and ForceCoeffs. There are a lot of interesting one I noticed (interesting as I've never really looked deeply at whats on offer). I think if we jot down our thoughts on the ROADMAP or somewhere else, it will might make minimise rework etc (esp if the "idea" comes from the other party).

Icon looks cool. Only comment is that the others are PNG and its generally good to standardise formats (or have sets of formats depending on OS's etc). Most OS's I have worked on use PNG or ICO (which is layered PNG's). I also noted the icons are all different sizes (I tried to standard to options on what seemed to be the most common h x w for the existing ones) and we need to standardise the naming convention, I just didnt get time to do that. Only comment is whether we'd want to standardise on the colours (dark grey / green / while / blue) or include reds as well. Subjective I guess. I think I'd personally standardise on a single format, potentially PNG.

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 04:35

Commented on core/gui/TaskPanelCfdFunctionObjects.ui line 1

:thinking: OK, i might have gotten confused - I thought you mentioned your GUI builder was non-configurable which is why I changed to what you were (are) using, so I assumed it would give what we agreed. I have not found a preference to change the indent in Qt Creator (same as you, unless you know different?)

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 04:40

Commented on core/plotters/CfdForceCoeffPlot.py line 45

As with the Porous zones (auto-enablement) etc, I've tried to break the back of the bulk of the functionality to start with rather than cover every nook and cranny and potential refinement - feel free to pull and make little changes like these if required. Happy for any to add to and / or refine / make better. So I guess more of a team relay / collaboration rather than individual submissions. In this instance, I've tried to do the bulk of the heavy lifting if that makes sense but obviously time is a constraint as well.

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 04:41

Commented on core/functionobjects/CfdFunctionObjects.py line 86

Agreed, will remove

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 05:34

Commented on core/functionobjects/CfdFunctionObjects.py line 134

thanks!

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 06:22

Commented on core/functionobjects/CfdFunctionObjects.py line 86

changed this line in version 9 of the diff

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 06:22

Commented on core/functionobjects/CfdFunctionObjects.py line 120

changed this line in version 9 of the diff

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 06:22

Commented on core/functionobjects/CfdFunctionObjects.py line 134

changed this line in version 9 of the diff

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 06:22

added 1 commit

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 10:06

Commented on data/defaults/system/controlDict line 111

changed this line in version 10 of the diff

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 10:06

added 1 commit

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 10:14

added 4 commits

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 13:55

added 1 commit

Compare with previous version

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 13, 2022, 14:04

Cool. To clarify, my comment was mostly just directed at the naming - I would use CfdReportingFunctions rather than CfdFunctionObjects, as I think you'll need a different interface for other types of function objects.

We started with png icons but as FreeCAD supports svg, which is infinitely/losslessly scalable, this seems like the way to go and I am in the process of transitioning over. It means re-drawing the old icons though, so not will take a while.

Yup, I wondered about the colour scheme as well. We can try a few things and see what looks best.

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 13, 2022, 14:11

Commented on core/gui/TaskPanelCfdFunctionObjects.ui line 1

That's right, I was asking for one space initially but as you said you preferred larger indents I was offering to reformat to four spaces (https://gitlab.com/opensimproject/cfdof/-/merge_requests/10#note_902014601). But if you're fine with tolerating the single space indents then we don't have to worry about reformatting so all good with me...

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 13, 2022, 14:15

approved this merge request

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 13, 2022, 14:25

Thanks for another great contribution. Sorry if I offended - I only meant to offer my perspective.

Perhaps it's better if you work directly in develop and pop me a quesition if you want my input on something.

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 14:26

OK, got it. I'll change the name :thumbsup:

Re: SVG - ok cool. Should we standardise a (default) template size (maybe 512 x 512)?

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 14:28

Commented on core/gui/TaskPanelCfdFunctionObjects.ui line 1

OK cool - yeah, i did (do) but it doesnt seem like you can set this in Creator. Now that I am using Creator (not editing the XML), is not really that important from my side.

oliveroxtoby commented 1 year ago

In GitLab by @icojb25 on Apr 13, 2022, 14:30

Commented on CfdRunnableFoam.py line 200

I've removed the additional plotters, they all use the original Residual Plot class now? LMK you're good with that. Unless we're going to do something very different (like subplots) I think this should be fine?

oliveroxtoby commented 1 year ago

In GitLab by @oliveroxtoby on Apr 14, 2022, 13:44

Sure, sounds good.