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

[PL/CT] MPL for component transport process #2646

Merged
merged 10 commits into from Sep 9, 2019

Conversation

@renchao-lu
Copy link
Member

renchao-lu commented Sep 3, 2019

  • Material properties involved in the component transport process all invoked from MPL.
  • Linear property extended for linear dependency on multiple variables.
  1. Feature description was added to the changelog
  2. Tests covering your feature were added?
  3. Any new feature or behavior change was documented?
@@ -53,9 +46,7 @@ struct ComponentTransportProcessData
{
}

MaterialLib::PorousMedium::PorousMediaProperties porous_media_properties;
ParameterLib::Parameter<double> const& fluid_reference_density;

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 3, 2019

Member

The fluid reference density seems to be also unused.

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 3, 2019

Author Member

I think so too. Shall I ?

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 5, 2019

Author Member

Removed.

@@ -179,7 +186,7 @@ std::unique_ptr<MaterialPropertyLib::Property> createProperty(
//! \ogs_file_param{properties__property__Parameter__parameter_name}
config.getConfigParameter<std::string>("parameter_name");
auto const& parameter = ParameterLib::findParameter<double>(
parameter_name, parameters, 1, nullptr);
parameter_name, parameters, 0, nullptr);

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 4, 2019

Member

What is the reason of this change?

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 5, 2019

Author Member

The parameter found in the subtree <parameters>/<parameter> can be read anyway, irrespective of number of components.

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 5, 2019

Member

@renchao-lu That is true. So the change isn't necessary?

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 5, 2019

Member

@TomFischer 0 means, that the number of components is arbitrary.

: porous_media_properties(std::move(porous_media_properties_)),
fluid_reference_density(fluid_reference_density_),
fluid_properties(std::move(fluid_properties_)),
: fluid_reference_density(fluid_reference_density_),
media_map(std::move(media_map_)),
decay_rate(decay_rate_),

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 4, 2019

Member

Shouldn't the decay rate also be handled as MPL property?

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 5, 2019

Author Member

Technically viable. I am not sure it proper to consider decay rate as part of material physical properties.

This comment has been minimized.

Copy link
@montoyav

montoyav Sep 5, 2019

Member

In my opinion decay should not be consider as material property as this property is specific of radioisotopes or what we consider "components".

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 5, 2019

Member

If I remember correctly, the decay rate was used for tracers too.

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 5, 2019

Member

Whether the decay rate goes into MPL or not, depends on where and how it is used. I didn't look into the details, so no suggestions for now.

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 5, 2019

Member

When the decay rate is a property of a component we should add it as a MPL component .

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 5, 2019

Author Member

Now it turns clear to me. Decay rate should be under the subtree of aqueous liquid phase and assigned as component property.

This comment has been minimized.

Copy link
@montoyav

montoyav Sep 5, 2019

Member

If I remember correctly, the decay rate was used for tracers too.

If tracers are not considered components, then decay it is also relevant for tracers.

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 6, 2019

Author Member

Decay rate invoked from MPL.

<type>Parameter</type>
<parameter_name>constant_porosity_parameter</parameter_name>
</property>
<property>

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 4, 2019

Member

Is the storage used anywhere? If not you could remove this tags from all project files.

This comment has been minimized.

Copy link
@renchao-lu

renchao-lu Sep 5, 2019

Author Member

Dropped.

Remark: Just retain one in the project file of Theis benchmark. In the project file, it is documented how to consider storage in an indirect way.

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented Sep 4, 2019

For discussion:
In this PR the permeability is handled as a property of the media, not as a property of the solid phase. In contrast to this in the HT process the permeability is a solid phase property. The approach in this PR is probably physical more correct?! To keep it consistent should I change the HT process also?

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented Sep 4, 2019

Project file tag documentation has to be updated.

@renchao-lu renchao-lu force-pushed the renchao-lu:MPLComponentTransportProcess branch from 87edbec to 69cd9ab Sep 4, 2019
@renchao-lu renchao-lu mentioned this pull request Sep 4, 2019
1 of 3 tasks complete
@renchao-lu

This comment has been minimized.

Copy link
Member Author

renchao-lu commented Sep 5, 2019

Project file tag documentation has to be updated.

Updated.

@renchao-lu renchao-lu force-pushed the renchao-lu:MPLComponentTransportProcess branch 3 times, most recently from 7e388c7 to 07c024f Sep 5, 2019
@renchao-lu

This comment has been minimized.

Copy link
Member Author

renchao-lu commented Sep 6, 2019

For discussion:
In this PR the permeability is handled as a property of the media, not as a property of the solid phase. In contrast to this in the HT process the permeability is a solid phase property. The approach in this PR is probably physical more correct?! To keep it consistent should I change the HT process also?

i think so.

@renchao-lu renchao-lu force-pushed the renchao-lu:MPLComponentTransportProcess branch from 07c024f to 685eb66 Sep 6, 2019
@renchao-lu renchao-lu force-pushed the renchao-lu:MPLComponentTransportProcess branch from 685eb66 to 812932b Sep 9, 2019
@endJunction endJunction merged commit d1a9b00 into ufz:master Sep 9, 2019
2 of 3 checks passed
2 of 3 checks passed
ufz.ogs in progress
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details
@renchao-lu renchao-lu deleted the renchao-lu:MPLComponentTransportProcess branch Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.