Repeated (similar) enum values from opm-parser in the rest of OPM

[blattms]

Hi,

While working on PR #684 I noticed again that there are a lot of enums defined in the rest of OPM that also exist (at least in a similar way) already in opm-parser. To construct the former we always use a switch statement on the original enum. Somehow this seems like a lot of unneeded branching and code to me.

Take for example the following:

from opm/parser/eclipse/EclipseState/Schedule/ScheduleEnums.hpp

enum Opm::WellCommon::WellProducer::ControlModeEnum{
            ORAT =     1,
            WRAT =     2, 
            GRAT =     4,
            LRAT =     8,
            CRAT =    16,
            RESV =    32,
            BHP  =    64, 
            THP  =   128, 
            GRUP =   256,  
            CMODE_UNDEFINED = 1024   
        };

from opm/core/wells/WellsManager_iml.hpp:

WellsManagerDetail::ProductionControl::Mode{ORAT, WRAT, GRAT,
                    LRAT, CRAT, RESV,
                    BHP , THP , GRUP };

from opm/core/wells/WellsManager.cpp:

Mode  Opm::InjectionControl::mode(Opm::WellInjector::ControlModeEnum controlMode)
        {
            switch ( controlMode  ) {
            case Opm::WellProducer::ORAT:
                return ORAT;
            case Opm::WellProducer::WRAT:
                return WRAT;
            case Opm::WellProducer::GRAT:
                return GRAT;
            case Opm::WellProducer::LRAT:
                return LRAT;
            case Opm::WellProducer::CRAT:
                return CRAT;
            case Opm::WellProducer::RESV:
                return RESV;
            case Opm::WellProducer::BHP:
                return BHP;
            case Opm::WellProducer::THP:
            default:
                throw std::invalid_argument("unhandled enum value");
            }

At least in this special case the only feature here is checking that the enum is not CMODE_UNDEFINED. (But maybe I am missing something here.) What I am wondering is, if this check is not done by opm-parser anyway.

This could be achieved a lot easier (if we want to keep the different enums).

We just make sure that the values are the same and skip the undefined one:

WellsManagerDetail::ProductionControl::Mode{
    ORAT = Opm::WellProducer::ORAT, 
    WRAT = Opm::WellProducer::WRAT,
    GRAT = Opm::WellProducer::GRAT,
        LRAT  = Opm::WellProducer::LRAT,
    CRAT = Opm::WellProducer::CRAT,
    RESV = Opm::WellProducer::RESV
        BHP = Opm::WellProducer::BHP,
    THP = Opm::WellProducer::THP,
    GRUP = Opm::WellProducer::GRUP };

and use a simple static cast for the conversion:

 Mode  Opm::InjectionControl::mode(Opm::WellInjector::ControlModeEnum
 controlMode){
       if(controlMode==CMODE_UNDEFINED)
         throw std::invalid_argument("unhandled enum value");
       return static_cast<Mode>(controlMode);
}

Being a dardevil, might I even suggest to get rid of the duplicate ones in OPM that only exist for historic reasons (read pre-opm-parser times).

BTW: I am aware that there are other enums that are a little bit different (e.g. 0 starting in core, and starting with 1 in dune-cornerpoint). Still I would think that for most of them there is a formula to convert between them that should be used rather than a switch statement.

[andlaus]

I agree with your issue, but you should be aware that it stems from a combination of historical baggage and dependency inversion: opm-parser is much newer than the wells code in opm-core so it could not have used the opm-parser enums when it was written. On the other hand opm-parser currently cannot use anything in opm-core because of the dependency order so new enums needed to be created. If you want to reduce these redundancies: perfect! patches are very welcome :)

[joakim-hove]

I agree that this is weird - and in my opinion the enums in opm-parser should be used. At the time opm-parser was shoehorned in we tried to make the changes as non-obtrusive as possible for the other modules; that is at least the recent background.

[atgeirr]

Still valid, not resolved, not working on it, will keep open as a reminder but remove from release milestone.