-
Notifications
You must be signed in to change notification settings - Fork 111
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
Make co2 non-static #4260
base: master
Are you sure you want to change the base?
Make co2 non-static #4260
Conversation
10000c8
to
312559c
Compare
@@ -0,0 +1,31 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license and copyright header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
#define OPM_CO2HELPER_STRUCTS_HPP | ||
|
||
#include <vector> | ||
struct TabulatedDensityTraits { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No namespace?
Also no whitespace (above the struct def.)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -0,0 +1,31 @@ | |||
|
|||
#ifndef OPM_CO2HELPER_STRUCTS_HPP | |||
#define OPM_CO2HELPER_STRUCTS_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No relation between:
- filename
- include guard macro name
- types defined in file
Please improve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated headerguard to match filename, not sure what you want from the name of the structs
{ | ||
public: | ||
CO2Parameters(){ | ||
co2Tables_ = CO2Tables(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: bad an d in con sistentformatting (2-8 spaces indent!), apply clang-format to new files as a rule of thumb.
More importantly: why a separate CO2Parameters class when CO2Tables could be used directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All three new files have run clang-format now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CO2Parameters now bypassed and using CO2Tables directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very good start, we need to discuss how to access the tables in the BrineCO2FluidSystem though.
*/ | ||
|
||
#ifndef OPM_CO2PARAMETERS_HPP | ||
#define OPM_CO2PARAMETERS_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Macro and filename mismatch. I would also prefer a blank line after the macro define.
module for the precise wording of the license and the list of | ||
copyright holders. | ||
*/ | ||
#include "CO2tables.inc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer regular includes/paths, even for "private" ones that do not require redistribution for library use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
/*! | ||
* \brief The dynamic viscosity [Pa s] of CO2. | ||
* | ||
* This is only here so that the API is valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of these functions added, can we not just remove them? What would be the consequence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing these functions causes compilation errors, seems related to the common interface described in "Components.hpp", althought I am not sure why it does not just inherit that implementation which just throws an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And having added new versions of these that also take in the parameter objects it is a bit ugly as inheriting the interface from components does not make it obvious that you are supposed to create a CO2Tables object and send that in instead, not sure how to clean this up nicely
@@ -50,17 +50,19 @@ Co2GasPvt(const std::vector<Scalar>& salinity, | |||
setActivityModelSalt(activityModel); | |||
setThermalMixingModel(thermalMixingModel); | |||
|
|||
co2Tables = Params(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will not this just do the entire table building over again, since it already would be default-initialized at this point?
Also in BrineCo2Pvt.cpp.
* | ||
* \return A const reference to the CO2 parameters. | ||
*/ | ||
const Params& getCo2Tables() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just for debugging? Can it be removed? If not, perhaps also add it to the Co2GasPvt class.
const float CO2<float>::brineSalinity; | ||
template<> | ||
const double CO2<double>::brineSalinity; | ||
// template<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just set the value inside the CO2 class, I dont think these lines are needed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll readd them iff the warnings are still there in jenkins
@@ -305,7 +306,8 @@ class BrineCO2FluidSystem | |||
} | |||
|
|||
assert(phaseIdx == gasPhaseIdx); | |||
LhsEval result = CO2::gasViscosity(temperature, pressure); | |||
CO2Tables params; // TODO: avoid this initialization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the TODO...
We must find a solution to this (all of the similar TODO places here) before it can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved with a static singleton. This will not cause problems for the GPUification of the code as Flow does not use this fluid system regardless.
e1cd51f
to
5880346
Compare
dd10037
to
fbec2db
Compare
Works with new tables generated by OPM/opm-utilities#87 |
jenkins build this please |
Static classes, in particular static buffers are causing some troubles in the GPU porting of OPM. This PR seeks to avoid the usage of a static implementation of the CO2 class.
WIP