Add control to fld output from a field list#2300
Add control to fld output from a field list#2300timofeymukha wants to merge 5 commits intoExtremeFLOW:developfrom
Conversation
|
Also adds a test! |
| end if | ||
| end if | ||
|
|
||
| if (idx .le. n_fields) then |
There was a problem hiding this comment.
This is just a suggestion but could we, instead of using the "skip_*" assign where to write based on a list?
Say I want to output just a variable that is the pressure in the user file or when post-processing etc. If we could just call my one filed "p" and then this assigns directly to P, then it would be nice. And if the name is not u,v,w,p,temp, then it is a scalar. That's a way I have found very useful but of course everyone has different taste :)
There was a problem hiding this comment.
Well, you can do that now, but if you pass fld_data instead of a field_list. The "only" issue is that fld_data owns its data, so you would have to make a copy of your fields into its vector_t components :(.
I personally think name matching brings in too many subjective options, e.g. p -> P, but what about pressure -> P, etc.? I would personally just treat .fld as a datadump, with the naming being random, with the exception of the default fluid output of velocity and pressure.
But maybe we can find a way to provide the mapping of names explicitly, in addition to the skip capability..
There was a problem hiding this comment.
Yeah, that's fair. In the end it is true. That name is only meaningful for 5 fields haha.
| if (.not. this%skip_pressure) then | ||
| if (idx .le. n_fields) then | ||
| p%ptr => data%items(idx)%ptr%x(:,1,1,1) | ||
| write_pressure = .true. | ||
| idx = idx + 1 | ||
| end if | ||
| end if | ||
|
|
||
| if (.not. this%skip_velocity) then | ||
| if (idx + 2 .le. n_fields) then | ||
| u%ptr => data%items(idx+0)%ptr%x(:,1,1,1) | ||
| v%ptr => data%items(idx+1)%ptr%x(:,1,1,1) | ||
| w%ptr => data%items(idx+2)%ptr%x(:,1,1,1) | ||
| write_velocity = .true. | ||
| idx = idx + 3 | ||
| end if | ||
| end if |
There was a problem hiding this comment.
I believe we have to be careful here, the whole select case(data%size()) block that you removed may be more important than we think :)
What happens if the field list only has 3 components? From what I can see you will have
p%ptrassociated, then- skip the velocity block, then
- enter temperature block,
temp%ptris associated, and - enter the scalars block where
scalar_fieldshas 1 entry that is associated.
So the resulting fld file would have a header PTS01 instead of U previously. So if I want it to be all in ´U´ I have to manually set this%skip_pressure? I don't know if that's an intended behavior. It kind of depends on the interpretation of the fields themselves, who says it shouldn't be S03 instead?
Let's say you want to output vorticity from the curl simcomp in a separate file. What happens then? Can/should you tell the field writer to skip pressure/velocity? Does that make sense?
There was a problem hiding this comment.
Yeah, I think manually setting the skip_* flags to get the behavior you want is the cumbersome part. That is why I thought the list would be good, but as mentioned, there are problems with that too.
There was a problem hiding this comment.
So if I want it to be all in ´U´ I have to manually set this%skip_pressure
Correct. My idea with this PR was to provide the option to choose what happens, instead of relying on a hard-coded convention. As you write, who says 3 fields should not be written as S03? Now you can decide what suits you. The only thing I don't like is that the default behavior of the code is changed here.
The question of how to use the new capability comes next, i.e. what fields to skip and when. I am personally leaning towards u,v,w,p,temp should only be used for the fluid's output, temp only in the compressible case. Everything else should be scalars. There is an argument to be made for U in the case of vector fields like curl, since you then get it as a vector field in paraview, which is convenient. As you write, the skip_ stuff can also be propagated in json. Whether that is a good idea is also a good question :). But I certainly like having this in user files for custom fld_file_t. But the whole thing is certainly all up to discussion.
There was a problem hiding this comment.
I agree that we should move away from hard-coded convention, and in general we should move towards that everything should be scalars except for fluid output. I can also see that a next step could be to set skip_temperature as true by default to move away from using temperature except for compressible cases, that's quite nice :D
What bothers me most is that the flags skip_* cause unpredictable behaviors, like with the example I mentioned where you'd pass down a list of three fields. In that example the velocity is skipped regardless of the value of skip_velocity, and a user might wonder why velocity was skipped even if set to false.
A suggestion could be:
- Define the "natural" order of fld writing as: p,u,v,w,t,s1,...,sN. In other words, remove the check
if (idx + 2 .le. n_fields) thenin the velocity block, so that weird behavior with skipping velocity doesn't happen. That way the order in which fields are written is exactly the same and is always predictable. - Set all the skip_* flags to
trueby default, in other words set the default behavior to dump only scalars. The fluid output and stats output being exceptions, they can set the relevant flags back tofalseas required and not break anything.
So then this would effectively:
- impose that everything is always dumped as a scalar, in the same order as it was passed down to fld_write,
- still give enough flexibility to not break the fld output,
- make the fld writer behavior consistent and predictable
- make the
skip_*flags actually work as intended, for example if I have 5 fields and skip_velocity is true, it would write p,t,s01,s02,s03. It's stupid simple and does exactly what it is instructed to do
There was a problem hiding this comment.
Wait, if (idx + 2 .le. n_fields) is there because we cannot just put things in u and leave v, w empty, right? Velocity is 3 fields or nothing. Or is that not true?
There was a problem hiding this comment.
It is true. it is not possible to have only one entry in the velocity. So that check needs to be there or we will get errors.
But I see the point victor makes. A user might expect that things are filled in order (they might not know). Then again, the current way of doing it is also filled with guesswork.
There was a problem hiding this comment.
oh sorry i completely forgot about that! My bad! that is so annoying.
on the other hand isn't that one more reason to just dump everything into scalars all the time? I'd say that if we can have all the skip_* flags to true by default and just have fluid/stats output mess with the flags to properly write pressure and velocity it would be great! Then you would barely change anything to the PR :D
There was a problem hiding this comment.
Let's dwell on this until next Tuesday I guess. I am also interested in what @tuananhdao thinks. The reason I started all of this was to potentially change how we work with temperature. But I also like the approach of just putting things to scalars by default.
|
Was this discussed in the development meeting? |
Yes, @timofeymukha will investigate a bit if we can make the default just a list of scalars. |
This adds components to the fld_file_t, which can be set to skip certain fields in the output when writing a passed field_list_t or field_t.
For example, setting skip_pressure to true, will make the solver skip writing into the p field. I stress that this is about the fields in the sense of "parts of the fld file", not in any physical sense; we simply adjust the mapping between the item number in the field_list_t to the locations in the fld file.
The mechanism is not used anywhere now, but it will allow us to adjust things if we decide to. It also provides this additional freedom to be used in the user file.
I isolated the logic into a subroutine,so that I can reuse it for field_t by creating a dummpy list with 1 field.