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

Improve error handling in MPL #2941

Merged
merged 4 commits into from May 31, 2020

Conversation

endJunction
Copy link
Member

@endJunction endJunction commented May 9, 2020

Improves error handling upon access of wrong type.
For the extra information on the medium/phase/component the definition scale was moved to the property.

Closes #2933
Review commit-wise.

  1. Feature description was added to the changelog

TODO:

  • Update other properties beside the Constant.

@endJunction
Copy link
Member Author

@joergbuchwald Please try it on your project files/processes.

@endJunction endJunction force-pushed the ImproveErrorMessagesInMPL branch 2 times, most recently from 20e47a0 to 07eed89 Compare May 11, 2020 11:45
@endJunction endJunction marked this pull request as ready for review May 11, 2020 12:42
@TomFischer
Copy link
Member

Is the TODO for a later PR?

Copy link
Member

@joergbuchwald joergbuchwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a substantial improvement. What would still be nice I think is, if we can also refer to the medium and phase in the error message.

@endJunction endJunction marked this pull request as draft May 12, 2020 21:13
@endJunction
Copy link
Member Author

Is the TODO for a later PR?

@TomFischer No. Done. Added names to all properties.

@@ -18,7 +18,9 @@
namespace MaterialPropertyLib
{
SaturationDependentSwelling::SaturationDependentSwelling(
std::array<double, 3> swelling_pressures,
std::string name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs clang format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array arguments are formatted differently.... Not nice, but consistent ;)

Copy link
Member

@TomFischer TomFischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice improvements! ⏩

@endJunction endJunction force-pushed the ImproveErrorMessagesInMPL branch 4 times, most recently from beefcc5 to 6af571b Compare May 13, 2020 13:12
@endJunction endJunction marked this pull request as ready for review May 13, 2020 14:40
Copy link
Member

@TomFischer TomFischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new parts are also ok.

Copy link
Member

@joergbuchwald joergbuchwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether it worked correctly.
I started to test some cases within the <phase> part:
I would expect especially the last cases to return errors.
Also, the thermal conductivity together with an IdealGasLaw should not throw a segfault.

<property>
                              <name>thermal_conductivity</name>
~                             <type>Contant</type>
                              <value>0.6</value>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/CreateProperty.cpp:143 createProperty()
error: The specified component property type 'Contant' was not recognized
info: OGS terminated on 2020-05-14 15:31:16+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>0.6 0.2</value>
                          </property>
warning: There are still some matrices and vectors in use. This might be an indicator of a possible waste of memory.
error: The value of property 'thermal_conductivity' defined for phase 'AqueousLiquid' is not of the requested type 'd' but a 2-vector.
info: OGS terminated on 2020-05-14 15:33:21+0200.
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>0.6 0.2 0.4</value>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Property.h:130 value()
warning: There are still some matrices and vectors in use. This might be an indicator of a possible waste of memory.
error: The value of property 'thermal_conductivity' defined for phase 'AqueousLiquid' is not of the requested type 'd' but a 3-vector.
info: OGS terminated on 2020-05-14 15:34:33+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>0.6 0.2 0.4 0.3</value>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Property.h:130 value()
warning: There are still some matrices and vectors in use. This might be an indicator of a possible waste of memory.
error: The value of property 'thermal_conductivity' defined for phase 'AqueousLiquid' is not of the requested type 'd' but a 2x2-matrix.
info: OGS terminated on 2020-05-14 15:35:12+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>0.6 0.2 0.4 0.3 0.1</value>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Property.cpp:57 fromVector()
error: Conversion of a 5-vector to PropertyDataType is not implemented.
info: OGS terminated on 2020-05-14 15:36:47+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>0.6 0.2 0.4 0.3 0.1 0.7</value>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Property.h:130 value()
warning: There are still some matrices and vectors in use. This might be an indicator of a possible waste of memory.
error: The value of property 'thermal_conductivity' defined for phase 'AqueousLiquid' is not of the requested type 'd' but a 3D-Kelvin vector.
info: OGS terminated on 2020-05-14 15:37:56+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>0.6 0.2 0.4 0.3 0.1 0.7 0.01</value>
                          </property>

critical: /home/joerg/ogs/MaterialLib/MPL/Property.cpp:57 fromVector()
error: Conversion of a 7-vector to PropertyDataType is not implemented.
info: OGS terminated on 2020-05-14 15:40:11+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>-0.6</value>
                          </property>
No Error!
--
<type>AqueousLiquid</type>
...
                            <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>-0.6</value>
                          </property>
 <type>Solid</type>
 ...
                           <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>-1.838</value>
                          </property>
Divergence
--
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                                         <!--<value>0.6</value>//-->
                          </property>
critical: /home/joerg/ogs/BaseLib/ConfigTree.cpp:235 onerror()
error: ConfigTree: In file `square_1e2_lin.prj' at path <media/medium/phases/phase/properties/property>: Key <value> has not been found
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>IdealGasLaw</type>
~                                         <!-- <value>0.6</value>//-->
                          </property>
info: === Time stepping at step #1 and time 5000 with step size 5000
info: Calculate non-equilibrium initial residuum.
[1]    58915 segmentation fault (core dumped)  ~/ogs/build-mkl/bin/ogs square_1e2_lin.prj
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>IdealGasLaw</type>
~                             <value>0.6</value>
                          </property>
critical: /home/joerg/ogs/BaseLib/ConfigTree.cpp:235 onerror() 
error: ConfigTree: In file `square_1e2_lin.prj' at path <media/medium/phases/phase/properties/property>: Key <value> has been read 1 time(s) less than it was present in the configuration tree.
info: Initialize processes.
error: ConfigTree: There have been errors when parsing the configuration file(s):
error: ConfigTree: In file `square_1e2_lin.prj' at path <media/medium/phases/phase/properties/property>: Key <value> has been read 1 time(s) less than it was present in the configuration tree.
critical: /home/joerg/ogs/BaseLib/ConfigTree.cpp:260 assertNoSwallowedErrors() 
error: There have been errors when parsing the configuration file(s).
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>Linear</type>
~                               <reference_value>0.4</reference_value>
+                               <independent_variable>
+                                   <variable_name>temperature</variable_name>
+                                   <reference_condition>293.15</reference_condition>
+                                   <slope>4e-4</slope>
+                               </independent_variable>
                          </property>
No error
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>Linear</type>
~                               <reference_value>0.4</reference_value>
+                               <independent_variable>
+                                   <variable_name>temperature</variable_name>
+                                   <reference_condition>293.15 0.2</reference_condition>
+                                   <slope>4e-4</slope>
+                               </independent_variable>
                          </property>
critical: /home/joerg/ogs/BaseLib/ConfigTree.cpp:235 onerror()
error: ConfigTree: In file `square_1e2_lin.prj' at path <media/medium/phases/phase/properties/property/independent_variable/reference_condition>: Value `293.15 0.2' is not convertible to the desired type.
info: OGS terminated on 2020-05-14 15:58:19+0200.
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>Linear</type>
~                               <reference_value>0.4 0 0</reference_value>
+                               <independent_variable>
+                                   <variable_name>temperature</variable_name>
+                                   <reference_condition>293.15</reference_condition>
+                                   <slope>4e-4</slope>
+                               </independent_variable>
                          </property>
critical: /home/joerg/ogs/BaseLib/ConfigTree.cpp:235 onerror() 
error: ConfigTree: In file `square_1e2_lin.prj' at path <media/medium/phases/phase/properties/property/reference_value>: Value `0.4 0 0' is not convertible to the desired type.
info: OGS terminated on 2020-05-14 15:59:33+0200.
error: OGS terminated with error.
--
<type>Solid</type>
...
                         <property>
                             <name>thermal_conductivity</name>
                             <type>Linear</type>
                               <reference_value>0.4 0 0 0 0.5 0 0 0 0.4</reference_value>
                               <independent_variable>
                                   <variable_name>temperature</variable_name>
                                   <reference_condition>293.15</reference_condition>
                                   <slope>4e-4</slope>
                               </independent_variable>
                           </property>
critical: /home/joerg/ogs/BaseLib/ConfigTree.cpp:235 onerror() 
error: ConfigTree: In file `square_1e2_lin.prj' at path <media/medium/phases/phase/properties/property/reference_value>: Value `0.4 0 0 0 0.5 0 0 0 0.4' is not convertible to the desired type.
info: OGS terminated on 2020-05-14 16:02:11+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>density</name>
                             <type>LinearProperty</type>
                              <reference_value>999.1</reference_value>
                              <independent_variable>
                                  <variable_name>temperature</variable_name>
                                  <reference_condition>273.15</reference_condition>
                                  <slope>-4e-4</slope>
                              </independent_variable>
                          </property>

critical: /home/joerg/ogs/MaterialLib/MPL/CreateProperty.cpp:143 createProperty()
error: The specified component property type 'LinearProperty' was not recognized
info: OGS terminated on 2020-05-14 16:05:31+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>Exponential</type>
~                             <reference_value>1.0e-3</reference_value>
+                             <exponent>
+                                 <variable_name>temperature</variable_name>
+                                 <reference_condition>20</reference_condition>
+                                 <factor>0.01333333</factor>
+                             </exponent>
                          </property>
No Error.
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>Dupuit</type>
~                             <parameter_name>kappa1</parameter_name>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Property.h:130 value() 
warning: There are still some matrices and vectors in use. This might be an indicator of a possible waste of memory.
error: The value of property 'thermal_conductivity' defined for phase 'AqueousLiquid' is not of the requested type 'd' but a 2x2-matrix.
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>BishopsPowerLaw</type>
~                             <exponent>1</exponent>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Properties/BishopsPowerLaw.cpp:24 checkScale() 
error: The property 'BishopsPowerLaw' is implemented on the 'media' scale only.
info: OGS terminated on 2020-05-14 16:33:56+0200.
error: OGS terminated with error.
--
                          <property>
                              <name>thermal_conductivity</name>
~                             <type>PorosityFromMassBalance</type>
~                             <initial_porosity>kappa1</initial_porosity>
+                             <minimal_porosity>0</minimal_porosity>
+                             <maximal_porosity>1</maximal_porosity>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Properties/PorosityFromMassBalance.cpp:30 checkScale()
error: The property 'PorosityFromMassBalance' must be given in the 'Solid' phase, not in 'AqueousLiquid' phase.
info: OGS terminated on 2020-05-14 16:35:17+0200.
error: OGS terminated with error.
--
<type>Solid</type>
..+                         <property>
+                               <name>thermal_conductivity</name>
+ ~                             <type>PorosityFromMassBalance</type>
+ ~                             <initial_porosity>0.4</initial_porosity>
+ +                             <minimal_porosity>0</minimal_porosity>
+ +                             <maximal_porosity>1</maximal_porosity>
+                           </property>
No Error!
--
<type>Solid</type>
+                         <property>
+                              <name>thermal_conductivity</name>
+                              <type>Parameter</type>
+                              <parameter_name>nu</parameter_name>
+                           </property>
No Error.
--
<type>Solid</type>
...
+                         <property>
+                              <name>thermal_conductivity</name>
+                              <type>BishopsPowerLaw</type>
+                              <exponent>0.3</exponent>
+                           </property>
No Error.
--
<type>Solid</type>
+                         <property>
+                              <name>thermal_conductivity</name>
+                             <type>RelativePermeabilityVanGenuchten</type>
+                             <residual_liquid_saturation>0.1689</residual_liquid_saturation>
+                             <residual_gas_saturation>0.05</residual_gas_saturation>
+                             <exponent>0.789029535864979</exponent>
+                             <minimum_relative_permeability_liquid>1e-12</minimum_relative_permeability_liquid>
+                          </property>

No Errror.
--
<type>Solid</type>
+                         <property>
+                              <name>thermal_conductivity</name>
+                             <type>PermeabilityOrthotropicPowerLaw</type>
+                             <intrinsic_permeabilities>0 1</intrinsic_permeabilities>
+                             <exponents>1 1</exponents>
+                          </property>
No Error.
--
<type>Solid</type>
..
+                         <property>
+                              <name>thermal_conductivity</name>
+                              <type>RelPermBrooksCorey</type>
+                     <residual_liquid_saturation>0.0</residual_liquid_saturation>
+                     <residual_gas_saturation>0.0</residual_gas_saturation>
+                     <lambda>2</lambda>
+                     <min_relative_permeability_liquid>0</min_relative_permeability_liquid>
+                     <min_relative_permeability_gas>1e-9</min_relative_permeability_gas>
+                          </property>
No Error

@endJunction
Copy link
Member Author

endJunction commented May 15, 2020

@joergbuchwald Could you provide an example which is failing, i.e. does not run as you would be expecting?

The IdealGasLaw case now produces readable error message.

@endJunction endJunction force-pushed the ImproveErrorMessagesInMPL branch 2 times, most recently from 9371fbf to 02e8a3c Compare May 16, 2020 20:27
@TomFischer
Copy link
Member

@joergbuchwald I have lost track of the extensive requests for changes. Are there still open questions?

@joergbuchwald
Copy link
Member

@joergbuchwald I have lost track of the extensive requests for changes. Are there still open questions?

I think there are some examples, where the output differs from the expected behavior. Afaik, the last commit was not directly related to the tests I performed.

@endJunction
Copy link
Member Author

endJunction commented May 18, 2020

I think there are some examples, where the output differs from the expected behavior.

Can you give us an example with unexpected behaviour?

Afaik, the last commit was not directly related to the tests I performed.

The last commit "[MPL] Phase property checked access." fixes the above mentioned exception if the IdealGasLaw is used without defining the molar_mass.

@joergbuchwald
Copy link
Member

joergbuchwald commented May 18, 2020

  1. The first case, I thought was not directetly intedend to be solved by this pull request, but it is related to the same issue:
<property>
                              <name>thermal_conductivity</name>
~                             <type>Contant</type>
                              <value>0.6</value>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/CreateProperty.cpp:143 createProperty()
error: The specified component property type 'Contant' was not recognized
info: OGS terminated on 2020-05-14 15:31:16+0200.
error: OGS terminated with error.
  1. The same holds, i guess for this case, one doesn't know where the error occured:
                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                             <value>0.6 0.2 0.4 0.3 0.1</value>
                          </property>
critical: /home/joerg/ogs/MaterialLib/MPL/Property.cpp:57 fromVector()
error: Conversion of a 5-vector to PropertyDataType is not implemented.
info: OGS terminated on 2020-05-14 15:36:47+0200.
error: OGS terminated with error.
  1. In the following cases, wrong types are used, but no error is thrown:
--
<type>Solid</type>
...
+                         <property>
+                              <name>thermal_conductivity</name>
+                              <type>BishopsPowerLaw</type>
+                              <exponent>0.3</exponent>
+                           </property>
No Error.
--
<type>Solid</type>
+                         <property>
+                              <name>thermal_conductivity</name>
+                             <type>RelativePermeabilityVanGenuchten</type>
+                             <residual_liquid_saturation>0.1689</residual_liquid_saturation>
+                             <residual_gas_saturation>0.05</residual_gas_saturation>
+                             <exponent>0.789029535864979</exponent>
+                             <minimum_relative_permeability_liquid>1e-12</minimum_relative_permeability_liquid>
+                          </property>

No Errror.
--
<type>Solid</type>
+                         <property>
+                              <name>thermal_conductivity</name>
+                             <type>PermeabilityOrthotropicPowerLaw</type>
+                             <intrinsic_permeabilities>0 1</intrinsic_permeabilities>
+                             <exponents>1 1</exponents>
+                          </property>
No Error.
--
<type>Solid</type>
..
+                         <property>
+                              <name>thermal_conductivity</name>
+                              <type>RelPermBrooksCorey</type>
+                     <residual_liquid_saturation>0.0</residual_liquid_saturation>
+                     <residual_gas_saturation>0.0</residual_gas_saturation>
+                     <lambda>2</lambda>
+                     <min_relative_permeability_liquid>0</min_relative_permeability_liquid>
+                     <min_relative_permeability_gas>1e-9</min_relative_permeability_gas>
+                          </property>
No Error

I guess, theese were the cases you inteded to cover?

@endJunction
Copy link
Member Author

  1. It is not related to this PR. But what is wrong with the error message? How can one improve it?

  2. The error location and the reason are specified rather well. What would you expect?

  3. The property names and the types are independent. This is intended behaviour.

@joergbuchwald
Copy link
Member

1. It is not related to this PR. But what is wrong with the error message? How can one improve it?

2. The error location and the reason are specified rather well. What would you expect?

1&2. in both cases the location, where the error is thrown is unclear.

3. The property names and the types are independent. This is intended behavior.

Okay, so I don't have a clue which cases you wanted to cover.
I thought this PR is about checking for wrong types and I thought types are wrong if they are associated with the wrong properties.

@endJunction
Copy link
Member Author

endJunction commented May 18, 2020

1. It is not related to this PR. But what is wrong with the error message? How can one improve it?

2. The error location and the reason are specified rather well. What would you expect?

1&2. in both cases the location, where the error is thrown is unclear.

Hmm, the errors say
critical: /home/joerg/ogs/MaterialLib/MPL/CreateProperty.cpp:143 createProperty()
critical: /home/joerg/ogs/MaterialLib/MPL/Property.cpp:57 fromVector()
These are precise locations of the error.
My question was more on how to improve the error messages:
error: The specified component property type 'Contant' was not recognized
and error: Conversion of a 5-vector to PropertyDataType is not implemented.

Okay, so I don't have a clue which cases you wanted to cover.
I thought this PR is about checking for wrong types and I thought types are wrong if they are associated with the wrong properties.

Ah, OK. There is slight misunderstanding. This PR is about the types stored in the property (scalar, vector, matrix, etc.) and access of those but for a wrong type. If porosity is a scalar but I want to get a vector out of it, it failed before with "unexpected index", now the property, its type (scalar, vector, etc.) is printed.

@joergbuchwald
Copy link
Member

Hmm, the errors say
critical: /home/joerg/ogs/MaterialLib/MPL/CreateProperty.cpp:143 createProperty()
critical: /home/joerg/ogs/MaterialLib/MPL/Property.cpp:57 fromVector()
These are precise locations of the error.
My question was more on how to improve the error messages:
error: The specified component property type 'Contant' was not recognized
and error: Conversion of a 5-vector to PropertyDataType is not implemented.
My suggestions would be:

'Contant' is not a proper property type for the property `thermal_conductivity`.

and

A 5-vector is not a proper data type for the property `thermal_conductivity`.

For me, it is clear that in both cases the error is not thrown because of that specific property.
('Contant' is not a known type for anything and 5-vectors a generally unknown ogs independent from the property )
But from the viewpoint of the users types are always related to property names.
However. an alternative approach would be to point to the xpath like in that example:

                          <property>
                              <name>thermal_conductivity</name>
                              <type>Constant</type>
~                                         <!--<value>0.6</value>//-->
                          </property>
critical: /home/joerg/ogs/BaseLib/ConfigTree.cpp:235 onerror()
error: ConfigTree: In file `square_1e2_lin.prj' at path <media/medium/phases/phase/properties/property>: Key <value> has not been found

@endJunction
Copy link
Member Author

@joergbuchwald The xpath output is a bigger topic/change. I opened an issue for it. Feel free to extend it as needed (just in case I missed the actual request ;)

@joergbuchwald
Copy link
Member

joergbuchwald commented May 18, 2020

@joergbuchwald I have lost track of the extensive requests for changes. Are there still open questions?

There are still some cases that we need to test.
I will do that in the upcoming days.

@endJunction
Copy link
Member Author

Part, which is unrelated to the error message correctness was merged into master in #2958. PR rebased. Waiting for remaining test cases.

@endJunction endJunction force-pushed the ImproveErrorMessagesInMPL branch 2 times, most recently from be5384a to 116dccb Compare May 29, 2020 15:23
@endJunction
Copy link
Member Author

@joergbuchwald If there are no more remarks, I'd like to merge the changes.

Also used for error reporting.
Accessing a wrong type in value(), dValue() etc.
functions is caught with a more or less descriptive
error. Keep in mind, the name of a type is compiler/STL
implementation dependent.
Also adjust placement of ticks for consistency.
@endJunction endJunction merged commit ee84a0c into ufz:master May 31, 2020
@endJunction endJunction deleted the ImproveErrorMessagesInMPL branch May 31, 2020 15:36
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants