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

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

Open
blattms opened this issue Nov 20, 2014 · 3 comments
Open

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

blattms opened this issue Nov 20, 2014 · 3 comments

Comments

@blattms
Copy link
Member

blattms commented Nov 20, 2014

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
Copy link
Contributor

andlaus commented Nov 20, 2014

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
Copy link
Member

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 atgeirr added this to the Release 2015.10 milestone Mar 25, 2015
@atgeirr
Copy link
Member

atgeirr commented Oct 21, 2015

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

@atgeirr atgeirr removed this from the Release 2015.10 milestone Oct 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants