simphony / simphony-metadata

[LEGACY] This repository contains the metadata definitions used in SimPhoNy project.
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Add entries for PDE conditions #121

Closed ahashibon closed 7 years ago

ahashibon commented 7 years ago

add missing value to conditions: a PDE condition needs a value, one variable, and an optional material (phase) it acts on. If no material is specified, it is assume that the condition applies universally to all materials.

ahashibon commented 7 years ago

@khiltunen @kemattil please check these changes mainly for PDE boundary conditions.

stefanoborini commented 7 years ago

After discussion offline, this PR can only be accepted with a considerable change in our specifications, parser, and overall vision. If we do so, we must do as indicated in issue #122

ahashibon commented 7 years ago

this is WIP. Just changed variables to have system scope for slip velocity. @stefanoborini would this work with current scope of the specs? if not will change back to "normal" CUBA.VARIABLE behaviour for now.

We are conducting small ontology analysis of boundary conditions, will update soon.

ahashibon commented 7 years ago

CUBA.VELOCITY is a CUBA.VARIABLE, for now what we have in cuba,yaml as "variables", and possibly other fields if defined even in simphony-metadata should be allowed for CUBA.VARIABLE. @khiltunen please comment.

Each condition works on a specific variable/field, and this has to be specified in the metadata. These, i.e., the variables and fields are part of the entities specified in the "variables" fixed property of Physics equations etc (which originally are are shortcuts for CUBA.VARIABLE with additional attribute being fixed on the metadata level, a note which did not transfer accurately enough probably to the specs here on github).

khiltunen commented 7 years ago

I agree with @ahashibon . For example for Dirichlet condition we must specify the VARIABLE for which the condition is set. Otherwise it is not known.

stefanoborini commented 7 years ago

CUBA.VELOCITY is a CUBA.VARIABLE

there's nothing in the current format that allows this kind of specification.

stefanoborini commented 7 years ago

@khiltunen the way this is normally done in the past is with the following writing

variables: [CUBA.FOO, CUBA.BAR, CUBA.BAZ]

khiltunen commented 7 years ago

Yes that's true. I can see your point now.

stefanoborini commented 7 years ago

In my opinion, and after analyzing the whole PR and discussing with Adham, my feeling is that:

  1. Either you create specific CUDS for the conditions, and therefore have specific variable properties for them, something that is in the spirit of an ontology. (e.g. a constant pressure condition has a PRESSURE variable, a constant volume has a VOLUME variable)
  2. or you define a generic condition (which is what is attempted here) and have yes, VARIABLE and VALUE. If that's the case, the following changes are required:

    1. VARIABLE must be a CUBA entry with type: cuba. Specs need to be expanded to cover this, and so is the compiler. Specifically, it allows only CUBA keys that are defined in cuba.yml.
    2. VALUE must be a CUBA entry with type: any. Specs need to be expanded to cover this, and so is the compiler. I would actually call it VARIABLE_VALUE There is no need to define a type to describe what is in VALUE. It can be inferred from VARIABLE.

    Additional requirements for the semantic correctness of the file is that if VARIABLE has a shape defined, VALUE must have the same shape and a default of compatible type (e.g. if VARIABLE is CUBA.PRESSURE, the VALUE default must not be a list of strings.)

With the second solution, you lose any information about the nature of the constraint. To know anything about it, you are forced to explore the specifics of what it contains. Unless you plan for hundreds of possible combinations leading to a proliferation of types, I think solution 1 is the most sensible, from a descriptiveness point of view.

ahashibon commented 7 years ago

@khiltunen @stefanoborini @mehdisadeghi This is still WIP but please check the current commits:

  1. each boundary condition is defined explicitly for each variable in the ontology/metadata
  2. added cuds for the volume fractions. This needs one more step:
    • currently a PHASE_VOLUME_FRACTION should be defined for each phase.
    • each entitiy (cell, face etc) can then have: entitiy.data[PHASE_VOLUME_FRACTION] = [PHASE_VOLUME_FRACTION_for phase 1, PHASE_VOLUME_FRACTION for phase 2, etc].

the question is whether there is a way to force the value for entitiy.data[PHASE_VOLUME_FRACTION] to be a list? or can we live with it as a convention for now?

stefanoborini commented 7 years ago

@ahashibon

CONSTANT_VOLUME_FRACTION_CONDITION:
    definition: Constant volume fraction condition
    parent: CUBA.DIRICHLET
    models: [CUBA.CONTINUUM]
    CUBA.VOLUME_FRACTION:
       shape: (:)
khiltunen commented 7 years ago

Maybe it is enough to let the wrappers to check that the entity.data[PHASE_VOLUME_FRACTION] value is a list. Though it would be nice if there were a way to generally check this. It is the same for all entity data for example entity.data[CUBA.VELOCITY] should be a list of size 3.

ahashibon commented 7 years ago

@stefanoborini can you please tell explain the issue with CONSTANT_VOLUME_FRACTION_CONDITION?

@khiltunen this is true for all cuba.yml entities, so it should be a convention by now to do these checks!

stefanoborini commented 7 years ago

@ahashibon that's how you express a list

ahashibon commented 7 years ago

@stefanoborini This is true, and would have been nice if VOLUME_FRACTION was already containing the phase information, or if we have a strict ordering of the phases. The problem is that such a list does not convey information to which phase each item in the CUBA.VOLUME_FRACTION list refers to. This is why we created the CUDS item PHASE_VOLUME_FRACTION, to hold both the phase and the volume fraction of that phase.

The alternative is to define in addition: PHASE_VOLUME_FRACTIONS: definition: volume fractions for a number of phases (material) on a dataset entity parent: CUBA.PHASE_VOLUME_FRACTION CUBA.PHASE_VOLUME_FRACTION: shape: (:)

(with plural S! or something similar), but this will complicate a bit the usage:

entity.data[PHASE_VOLUME_FRACTIONS].PHASE_VOLUME_FRACTION[0].material and entity.data[PHASE_VOLUME_FRACTIONS].PHASE_VOLUME_FRACTION[0].volume_fraction

instead of entity.data[PHASE_VOLUME_FRACTION].[0].material and
entity.data[PHASE_VOLUME_FRACTION].[0].volume_fraction

in both cases, the user/wrapper/visualisation tool etc, will definitely be able to infer/know to which phase (material) the index 0, etc, points too.

in CFD we have usually 2 phases for which one volume fraction is sufficient (sum must be 1), but in general for N phases we need N-1 volume fractions. Having the material explicitly defined allows us much needed flexibility, for example we can define a volume fraction of the mixture material in this way:

mixture=Material() v=PHASE_VOLUME_FRACTION(material=mixture, volume_fraction=1)

it is up to the wrapper/user then of course to make sure all is consistent, CUDS only provides the efficient data structure, the wrapper the "reasonning".

@khiltunen it is very tempting to also define:

MIXTURE_MATERIAL: definition: a mixture of few phases or materials parent: CUBA.MATERIAL CUBA.MATERIAL: shape: (:)

note: if the above would preserve the order of the phases, we could then easilly live with just CUBA.VOLUME_FRACTION as a list, but the way proposed in this commit is more "safe"!

... comments are very much welcome!

stefanoborini commented 7 years ago

What I am saying is that you defined PHASE_VOLUME_FRACTION and you never used it anywhere.

ahashibon commented 7 years ago

(update) maybe I missuderstood : PHASE_VOLUME_FRACTION should be used directly on the entities, and it does not need to be used, for now, in the metadata it self.

@stefanoborini we used it in the applications but not yet on any uploaded example. Also the lack of a clear way to relate it to a phase, meant we had to used ugly hacks! this is why we really need this urgently to make support for multiphase simulations possible in a standard clean manner!

@khiltunen please feel free to add your point of view ....

ahashibon commented 7 years ago

@mehdisadeghi @stefanoborini can this be merged already?

stefanoborini commented 7 years ago

@ahashibon There are review comments open and it needs merging of master.

ahashibon commented 7 years ago

@stefanoborini @kemattil please check, all review requests should be done!