search

OPM / opm-models

The models module for the Open Porous Media Simulation (OPM) framework
Other
17 stars 69 forks source link

removing compositional related from blackoilmodel.hh #844

Closed GitPaean closed 11 months ago

GitPaean commented 11 months ago

it is working in progress.

It aims at removing compostional related routines from blackoil related modules. There is a VtkBlackoil already.

GitPaean commented 11 months ago

jenkins build this please

GitPaean commented 11 months ago

jenkins build this please opm-simulators=4936 please

totto82 commented 11 months ago

You can compute many of the output you typically have in a compositional simulation from the blackoil model. Some of this conversion is implemented in the BlackoilFluidSystem, some just outputs trivial numbers. It may be useful to look at molar fractions etc even in a blackoil simulations if you want to compare it with compositional simulations. Why do you want to remove it?

GitPaean commented 11 months ago

With the current form, when developing https://github.com/OPM/opm-models/pull/838, when we added more output to the VtKCompostional, since VtkCompostional is included in the BlackoilModel, then we have to update the BlackoilFluidState when we add more output to the VtkCompostional. It might be a bad idea to keep filling the BlackoilFluidState with compostional related stuff, which causes confusion and maintenance burden.

Then I think it might be a good idea to make sure the VtkCompostional and VtkBlackoil be more separated. If not many compositional properties from Blackoil are needed to compare against the compositional results, maybe we can just put these few functions also in VtkBlackoil for now, and not include VtkCompostional in the BlackoilModel. Even for outputting purpose, those are still serving and based on blackoil simulations. It is okay to have them in the VtkBlackoil. It is also introduce some maintenance burden, but hopefully more manageable. At least, not when working on compositional simulation and have to take care of the blackoil related.

How do you think?

totto82 commented 11 months ago

I am fine with removing if it slow down the progress with the compositional model. I dont think many people uses the compositional vtk output from the blackoil model. If so we can just add what is needed into the blackoil vtk part.

GitPaean commented 11 months ago

Thanks. In the future, with some refactoring, we can do refactoring to reduce possible duplication.

GitPaean commented 11 months ago

@totto82 , are you okay with simply removing the including of the vtkcompostional from the blackoilmodel, or you prefer we can add the missing function to be vtkblackoil before removing?

I believe in the future, we can have another vtkmodule to handle the common one between compostional and blackoil models.

GitPaean commented 11 months ago

If we do not think vtkcompositionmodule.hh is for compsitional modeling. We think it is more composition information. Maybe we should keep vtkcmositionmodule while have a vtkcompostionalmodule.hh (or different name) to handle the compositional simulation related.

totto82 commented 11 months ago

are you okay with simply removing the including of the vtkcompostional from the blackoilmodel

Yes.

we should keep vtkcmositionmodule while have a vtkcompostionalmodule.hh (or different name) to handle the compositional simulation related.

I am find with this solution as well.

I suggest you do what you think is easiest in terms of getting something to work for the compositional runs. As I said the usage of compositional output from the blackoil model is probably not used by anybody. And if somebody needs it in the future it should be trivial to add i back.

GitPaean commented 11 months ago

Thanks for the discussion. It is in the files that I am not familiar with and some discussion is very helpful.

At the end, I decided to introduce a new module vtkptflashmodule to avoid touching vtkcompostionmodule.

In the future, there might be more refactoring.

I will close this PR now.