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

Implementation of HeatTransportBHE process #2221

Merged
merged 13 commits into from Nov 7, 2018

Conversation

Projects
None yet
5 participants
@HBShaoUFZ
Copy link
Contributor

HBShaoUFZ commented Sep 26, 2018

In this pull request, the HeatTransport BHE process is implemented.

TODO:

  • Fix compilation (@endJunction) compiles on jenkins now.
  • Provide project file tags' docu (WIP by Chaofan)

@HBShaoUFZ HBShaoUFZ force-pushed the HBShaoUFZ:bhe_impl branch 2 times, most recently from 59783a8 to b1ea3e6 Sep 26, 2018

@HBShaoUFZ HBShaoUFZ force-pushed the HBShaoUFZ:bhe_impl branch 2 times, most recently from e721c28 to 6b6b3ba Sep 27, 2018

@HBShaoUFZ HBShaoUFZ force-pushed the HBShaoUFZ:bhe_impl branch 2 times, most recently from 235283e to f8cfc21 Oct 1, 2018

@endJunction endJunction force-pushed the HBShaoUFZ:bhe_impl branch 2 times, most recently from 467d3fb to 60b7afa Oct 30, 2018

@HBShaoUFZ HBShaoUFZ force-pushed the HBShaoUFZ:bhe_impl branch from 60b7afa to 642ce17 Oct 30, 2018

}
// If only one of the global indices (in or out) is negative the
// implementation is not valid.
if (in_out_global_indices.first < 0 || in_out_global_indices.second < 0)

This comment has been minimized.

@TomFischer

TomFischer Nov 2, 2018

Member

Is my understanding correct that the way the domain is partitioned is important for a successful simulation run? Do you have an idea to avoid partitioning such that in_out_global_indices.first and in_out_global_indices.second are always on the same partition or is it an open issue?

This comment has been minimized.

@endJunction

endJunction Nov 2, 2018

Member

Yes. No. ;)

I added the checks just out of my head to be sure that partitioning does not go wrong. It still might go wrong, but for other reasons. I wouldn't try to prevent partitioning, but add communication, s.t. the values are passed between the partitions.

PipeConfiguration1U const& pipes)
: BHECommon{borehole, refrigerant, grout, flowAndTemperatureControl},
_pipes(pipes)
{

This comment has been minimized.

@TomFischer

TomFischer Nov 5, 2018

Member

Why aren't the implementations of the constructor and some other methods of this class in the cpp file?

This comment has been minimized.

@HBShaoUFZ

HBShaoUFZ Nov 5, 2018

Author Contributor

As suggested by TF, the implementation of the constructor and other functions have been moved to the cpp file. The one still staying in the h file is the "assembleRMatrices" function. This function is called in the constructor of local BHE element assembler to initialize the thermal resistance matrices.

1.0 * matBHE_loc_R; // K_ig
R_matrix.block(3 * NPoints, 3 * NPoints, NPoints, NPoints) +=
1.0 * matBHE_loc_R; // K_og
break;

This comment has been minimized.

@TomFischer

TomFischer Nov 5, 2018

Member

Why no default branch?

This comment has been minimized.

@HBShaoUFZ

HBShaoUFZ Nov 5, 2018

Author Contributor

A default case has been added to prevent the code from getting into undefined status.

{
namespace BHE
{
class BHE_1U final : public BHECommon

This comment has been minimized.

@TomFischer

TomFischer Nov 5, 2018

Member

Brief documentation of the class would be nice.

This comment has been minimized.

@HBShaoUFZ

HBShaoUFZ Nov 5, 2018

Author Contributor

Documentation has been added.

@HBShaoUFZ HBShaoUFZ force-pushed the HBShaoUFZ:bhe_impl branch from 642ce17 to 750b56a Nov 5, 2018

@TomFischer
Copy link
Member

TomFischer left a comment

Looks in general good. Some minor things to discuss/correct.

/// Distance between pipes.
double const distance;

double const longitudinal_despersion_length;

This comment has been minimized.

@TomFischer

TomFischer Nov 6, 2018

Member

Did you mean 'dispersion'?

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

It means the longitudinal thermal dispersivity, all the name has been replaced by the longitudinal_thermal_dispersivity.

This comment has been minimized.

@endJunction

endJunction Nov 6, 2018

Member

@ChaofanChen there is typo in the variable name...

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

Yes, but I don't think this variable name is clear enough, not just the typo problem. So I changed a lot including parameters' documentation.

This comment has been minimized.

@HBShaoUFZ

HBShaoUFZ Nov 6, 2018

Author Contributor

I will stick with "longitudinal_dispersion_length"

i;
}

_process_data._mesh_prop_materialIDs = material_ids;

This comment has been minimized.

@TomFischer

TomFischer Nov 6, 2018

Member

Minor: I would move line 65 behind line 56.

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

It has been fixed.

{
namespace HeatTransportBHE
{
using namespace BHE;

This comment has been minimized.

@TomFischer

TomFischer Nov 6, 2018

Member

@endJunction
I'm not sure if using namespace BHE; is here in the header okay.


BHEType const& _bhe;

std::size_t const element_id;

This comment has been minimized.

@TomFischer

TomFischer Nov 6, 2018

Member

Class attributes start with _.

This comment has been minimized.

@ChaofanChen
std::vector<ShapeMatrices, Eigen::aligned_allocator<ShapeMatrices>>
_shape_matrices;

std::size_t const element_id;

This comment has been minimized.

@TomFischer

TomFischer Nov 6, 2018

Member

Class attributes start with _.

This comment has been minimized.

@ChaofanChen
{
namespace HeatTransportBHE
{
/// Used by for extrapolation of the integration point values. It is ordered

This comment has been minimized.

@TomFischer

TomFischer Nov 6, 2018

Member

Used by for ...

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

The extra word has been deleted.

@HBShaoUFZ HBShaoUFZ force-pushed the HBShaoUFZ:bhe_impl branch from 750b56a to 1d19d6a Nov 6, 2018

@@ -0,0 +1 @@
The borehole diameter is one of the most important geometric parameters of borehole.

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

I think the explanation of a keyword should follow the style of what for i_borehole.md, which displays like the following


[tag] borehole

It represents the well used to install pipes for heat transport.

With the present form, the displayed one is

[tag]diameter

The borehole diameter is one of the most important geometric parameters of borehole.

It is better to change it to

[tag]diameter

It is the borehole diameter,  one of the most important geometric parameters of borehole.

or

[tag]diameter

The borehole diameter,  one of the most important geometric parameters of borehole.

The same for the following parameter documentations.

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

They are rephrased.

public:
BHEInflowDirichletBoundaryCondition(
std::pair<GlobalIndexType, GlobalIndexType>&& in_out_global_indices,
BHEType& bhe)

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

with const?

This comment has been minimized.

@endJunction

endJunction Nov 6, 2018

Member

Unfortunately not possible; The bhe has a global state which is updated in the getTinByTout call.
That member function (the name is not telling it) is not const and is modifying the flow rate and returns new temperature for the BC.

@HBShaoUFZ Could you please rename the function and its documentation, which mistakenly says something about "fixed power"?

*
* 1) Diersch_2011_CG
* Two very important references to understand this class implementations are:
* H.-J.G. Diersch, D. Bauer, W. Heidemann, W. R黨aak, P. Sch鋞zl,

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

It seems that there are Unicode characters in W. R ...aak, P. Sch...zl.

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

Corrected.

OGS_FATAL(
"Error!!! In the function BHE_1U::assembleRMatrices, "
"the index of bhe unknowns is out of range! ");
break;

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

This break is not needed.

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

It's been removed.

return {bhe_material_ids, bhe_elements, bhe_nodes};
}
} // end of namespace HeatTransportBHE
} // namespace ProcessLib

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

Now new line at the end of the file. It is not important but github complains at here.

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 7, 2018

Member

Corrected.

{
OGS_FATAL(
"The number of the given BHE properties (%d) are not consistent "
"with the number of BHE groups in a mesh (%d).",

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

a mesh --> the mesh

This comment has been minimized.

@ChaofanChen

ChaofanChen Nov 6, 2018

Member

Corrected.

auto const& K_ip = ip_data.dNdxTdNdx_product_times_w;

// auto const k_f = _process_data.thermal_conductivity_fluid(t, pos)[0];
// auto const k_g = _process_data.thermal_conductivity_gas(t, pos)[0];

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

Are k_f and k_g needed for future development?

This comment has been minimized.

@HBShaoUFZ

HBShaoUFZ Nov 6, 2018

Author Contributor

Yes. Currently this is the fully saturated case. I leave the k_f and k_g here in case later on RichardsFlow or MultiphaseFlow process in integrated with the HeatTransportBHE process. For now these parameters are commented out.


std::size_t const _element_id;

bool const _is_axially_symmetric;

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

Is _is_axially_symmetric needed? or another question, can the current implementation of BHE be applied for axis-symmetrical problem? I see that GlobalDim = 3.

This comment has been minimized.

@endJunction

endJunction Nov 6, 2018

Member

Removed member; Kept in ctor for shape function initialization.

1.0 * matBHE_loc_R; // K_i1
R_matrix.block(2 * NPoints, 2 * NPoints, NPoints, NPoints) +=
1.0 * matBHE_loc_R; // K_ig
break;

This comment has been minimized.

@wenqing

wenqing Nov 6, 2018

Member

break --> return

This comment has been minimized.

@ChaofanChen
@wenqing

wenqing approved these changes Nov 6, 2018

Copy link
Member

wenqing left a comment

Looks good. Only minor stuffs.

@HBShaoUFZ HBShaoUFZ force-pushed the HBShaoUFZ:bhe_impl branch from 1d19d6a to 08fff44 Nov 7, 2018

@endJunction endJunction merged commit 6cc64c7 into ufz:master Nov 7, 2018

2 of 3 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details
double R_gg, R_gs;
std::tie(R_gg, R_gs) = thermalResistancesGroutSoil(chi, R_ar, R_g);

return {R_fig, R_fog, R_gg, R_gs};

This comment has been minimized.

@wenqing

wenqing Nov 20, 2018

Member

Compilation warning:

BHE_1U.cpp:234:13: warning: suggest braces around initialization of subobject [-Wmissing-braces]
    return {R_fig, R_fog, R_gg, R_gs};
            ^~~~~~~~~~~~~~~~~~~~~~~~
            {                       }

Please change it to

return {{R_fig, R_fog, R_gg, R_gs}};

This comment has been minimized.

@endJunction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.