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

Question: equivalent of composite Python classes in C++ #695

Closed
ischoegl opened this issue Aug 15, 2019 · 10 comments · Fixed by #696
Closed

Question: equivalent of composite Python classes in C++ #695

ischoegl opened this issue Aug 15, 2019 · 10 comments · Fixed by #696

Comments

@ischoegl
Copy link
Member

ischoegl commented Aug 15, 2019

Cantera version

2.5.0a3

Expected Behavior

It would be useful to have C++ equivalents of composite classes that are defined in Python (i.e. Solution, Interface, etc.). A replication of those classes would clarify origin and structure of thermo/kinetics/transport managers that are instantiated from the Python composite class (the same holds true for Matlab).

This does not mean the replication of double/triple inheritance within C++. Rather, the C++ classes would receive pointers to previously allocated managers, as well as hold type information (which is currently lost in C++) and a uniquely assignable name.

The motivation is that a common root for composite objects appears to be necessary for serialization within the C++ layer. A direct mapping of Python to C++ objects would also clarify the overall structure of cantera.

Actual Behavior

There is no replication of composite classes within C++

Steps to reproduce

N/A

@speth
Copy link
Member

speth commented Aug 15, 2019

I've thought about this before, and while it would have some convenience, I feel like it would be extremely ugly -- there are over 300 methods to wrap among the Phase, ThermoPhase, Kinetics, and Transport classes.

I'm not sure I see what the problem is with serialization that requires this feature. I think all that is required is that the method which generates a complete phase definition needs to have the ThermoPhase, Kinetics, and Transport objects passed in, and the serialization method for each of them can add the relevant information to the generated data structure.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 15, 2019

there are over 300 methods to wrap among the Phase, ThermoPhase, Kinetics, and Transport classes.

There may be other ways to instantiate, but at least from Python there are only 3 classes: Solution, Interface and DustyGas. A suitable C++ class structure would have a base class, and three derived classes, with almost everything overloaded. I.e. it would be light-weight and hold the information together. Edit: I am not suggesting to wrap any methods; the only methods that are worth implementing are type and name plus (potentially) some casting of pointer types.

At the moment, the information is held together by objects that have sets of ThermoPhase, Kinetics and Transport managers passed to. Imho a solution that does not require accessing an object that is not related to either of those three in order to retrieve this info would be nice.

@ischoegl
Copy link
Member Author

Case in point: consider extract_submechanism.py

gas2 = ct.Solution(thermo='IdealGas', kinetics='GasKinetics',
                   species=species, reactions=reactions)

It may be useful to write the mechanism back to yaml without having to 'glue' the file together in python?

@speth
Copy link
Member

speth commented Aug 15, 2019

So you're thinking of a structure that just exposes the related ThermoPhase, Kinetics and Transport objects, rather than combining all of the methods the way the Python Solution class does? I can see that as providing a convenient way to instantiate all of the pieces of a phase. Something like:

class Solution {
public:
    Solution(const std::string& infile, const std::string& phasename); // and other constructors
    std::string name() const;
    unique_ptr<ThermoPhase> thermo;
    unique_ptr<Kinetics> kinetics;
    unique_ptr<Transport> transport;
};

A similar structure for Interface objects would certainly simplify instantiation of those phases in C++.

I'm not quite clear what you would expect the type() method to return, though.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 15, 2019

Almost, but I wouldn't even bother with loading the file there (I'd still use the existing approach. EDIT: I guess that is debatable as your thought may clarify things quite a bit). Also, apologies that my function signatures are likely incorrect (not using smart pointers much). I'm mostly Python ...

class Composite {
public:
    Composite(unique_ptr<ThermoPhase> thermo,
              unique_ptr<Kinetics> kinetics,
              unique_ptr<Transport> transport,
              const std::string& name="(none)"); // and other constructors
    std::string name() const;
    void setName(const std::string& name);
    virtual std::string type() const;
    unique_ptr<ThermoPhase> thermo;
    unique_ptr<Kinetics> kinetics;
    unique_ptr<Transport> transport;
};

class Solution : public Composite {
    virtual std::string type() const {
        return "Solution";
    }
};

class Interface : public Composite {
    virtual std::string type() const {
        return "Interface";
    }
};

class DustyGas : public Composite {
    virtual std::string type() const {
        return "DustyGas";
    }
};

PS: An alternative would be not set anything in the constructor and write setters for the managers instead. It really depends on how the __cinit__/__init__ are implemented in Python (and the matlab equilvalent).

PPS: the only thing I'd add in ThermoPhase, Kinetics and Transport managers is a pointer back to Composite. I believe this is all that is needed to 'hold information together'.

@ischoegl
Copy link
Member Author

And obviously I forgot the signature for a function std::string toYAML() ... 🤣

@speth
Copy link
Member

speth commented Aug 16, 2019

Oh, I see - you're after a way of writing a function that can instantiate a YAML phase entry without knowing whether it is for a Solution or an Interface (I'd leave DustyGas out of this for the moment -- it's kind of a wart already in that there's no way of instantiating it from an input file at all).

If the only polymorphic behavior is the return value of type, though, you don't really need to introduce a class hierarchy (which would require always working with Composite* objects, rather than just allocating them on the stack). Instead, you could either have the type function return a member variable containing the correct string (set during initialization) or determine it dynamically from the types of the thermo etc. objects. For example, it could be as simple as "if thermo->nDim() < 3 the type is Interface, otherwise, Solution".

@ischoegl
Copy link
Member Author

Yes, that pretty much is what I had in mind. You’re correct that polymorphism may not be required at all. I’d have to code things up ... things have a tendency to clear up in this process ...

@ischoegl
Copy link
Member Author

Looks like the name of Composite should really be SolutionBase. The diamond inheritance in Python cannot be avoided as Cython doesn’t allow for multiple inheritance.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 16, 2019

@speth ... coding things up did clarify things as anticipated. Feel free to comment on #696 ...

I also left a stub for your suggestion

 SolutionBase(const std::string& infile, const std::string& phasename);

as I believe that it would be a logical (but not necessary) next step.

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

Successfully merging a pull request may close this issue.

3 participants