coolfluid / coolfluid3

COOLFluiD is a collaborative simulation environment focused on complex multi-physics simulations
http://coolfluid.github.com
76 stars 77 forks source link

Interpretation of "basic" components and options in python #203

Closed barche closed 12 years ago

barche commented 12 years ago

Components and options that are marked as basic could show up in python as just a member option, i.e. suppose that a solver named 'NavierStokes' is marked as a basic component in model, you could access it as:

model.NavierStokes

instead of:

model.get_child('NavierStokes')

Same for options:

time.time_step = 0.1

instead of:

time.options().configure_option('time_step', 0.1)
tlmquintino commented 12 years ago

Yes this makes sense. Great feature to have...

wdeconinck commented 12 years ago

Yes this will add a whole new dimension!

Good luck for the person responsible to get this with the python bindings, and tab-completion ;-)

wdeconinck commented 12 years ago

Notice that I already implemented the first part of this issue, being the access of a component.

For the options, I would be more careful. There could be conflicts with component names and option names. It is maybe more wise to implement this as: time.options().time_step = 0.1 dt = time.properties().dt

Otherwise this could be confusing for users to know if something is a component or an option, or a property.

barche commented 12 years ago

Note that this issue was assigned to me, so at least talk before working on it, otherwise we do double work.

Option access is implemented in my tree, using a setattr override. At first, I didn't want to go for getattr and just add values as attributes directly (in the interest of tab completion, not sure if that works when getattr is overridden), but in the end I'm stuck with some recursions using that method, so I'll override getattr to get both options and basic components.

Since options and components both need to be marked as basic explicitly, and this only covers basic options and components, it's up to the component designer to avoid conflicts.

wdeconinck commented 12 years ago

Oh don't worry about my double work. It was basically mapping the "getattr" to get_child(), nothing major, 1 line of code. I was just curious to see how that would be done as proof of concept. I am sure that it required more work for tab-completion etc. This was just a temporary thing. If it was more work, I would for sure not have done this without prior warning.

About conflicts between options and components, I would not state a C++ component developer should worry about this, as this is a python issue. And a user can always create dynamically a component with the same name as an option too.

Maybe we can check for conflicts, and if so, explicitly warn the user about it, and inform to use a non-ambiguous alternative way to access the option or component.

barche commented 12 years ago

Yes, adding a warning would be easy to do. I'd propose to give priority to options, and if both an option and component exist, warn that we're choosing the option.

You're right that the programmer shouldn't worry about this, what I mean is that since it only concerns items marked as basic, these are part of the base interface of a component, and having an overlap would be unlikely, and we could provide it as a guideline to try and avoid it. Even just changing case (i.e. use a capital for the component) would resolve it.

wdeconinck commented 12 years ago

Or give the priority to component, and just put component.options().key = value to resolve the ambiguity in such case ? It looks cleaner than calling component.get_child('name') in my opinion as it doesn't involve a quoted string. Just my 2 cents.

wdeconinck commented 12 years ago

Very nice! I already tested it, and it works all-over, except for one problem:


# This one works
bc_farfield.options().set('constants' , [rho_inf,rho_inf*u_inf,0.,rhoE_inf])

# This one doesn't work yet
bc_farfield.constants = [rho_inf,u_inf,0.,rhoE_inf]

Strangely, once it has been configured with the first version, the second version also works. It is as if the second version doesn't resize the original option array or something. This problem doesn't show during the configuration, but during simulation time, trying to use the option values.

wdeconinck commented 12 years ago

I updated one atest in the main repo, namely "plugins/sdm/test/atest-sdm-euler-cylinder-2d.py" to the new style, and the lines mentioned in the previous post start at line number 79

wdeconinck commented 12 years ago

One more problem discovered with tab-completion:

Trying to use tab-completion on any component such as this

>>> import coolfluid
>>> coolfluid.root.<TAB>

results in an exception

+++ Exception thrown on rank 0 ++++++++++++++++++++++++++++++++++++++++++++++
From : '/Users/willem/workspace/coolfluid3/dev/kernel/cf3/python/ComponentWrapper.cpp:298:component_getattr'
Type : 'ValueNotFound'
Message :
Attribute __members__ does not exist as either a basic option or a basic component for object at /
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

+++ Exception backtrace on rank 0 ++++++++++++++++++++++++++++++++++++++++++
dumping 1 backtrace ...

backtrace() returned 58 addresses
cf3::common::MacOSX::OSystemLayer::back_trace() const
cf3::common::Exception::Exception(cf3::common::CodeLocation, std::string, std::string)
cf3::common::ValueNotFound::ValueNotFound(cf3::common::CodeLocation const&, std::string const&)
cf3::python::component_getattr(cf3::python::ComponentWrapper&, std::string const&)
boost::python::detail::caller_arity<2u>::impl<boost::python::api::object (*)(cf3::python::ComponentWrapper&, std::string const&), boost::python::default_call_policies, boost::mpl::vector3<boost::python::api::object, cf3::python::ComponentWrapper&, std::string const&> >::operator()(_object*, _object*)
boost::python::objects::function::call(_object*, _object*) const
boost::detail::function::void_function_ref_invoker0<boost::python::objects::(anonymous namespace)::bind_return, void>::invoke(boost::detail::function::function_buffer&)
boost::python::detail::exception_handler::operator()(boost::function0<void> const&) const
boost::detail::function::function_obj_invoker2<boost::_bi::bind_t<bool, boost::python::detail::translate_exception<cf3::common::FailedAssertion, void (*)(cf3::common::FailedAssertion const&)>, boost::_bi::list3<boost::arg<1>, boost::arg<2>, boost::_bi::value<void (*)(cf3::common::FailedAssertion const&)> > >, bool, boost::python::detail::exception_handler const&, boost::function0<void> const&>::invoke(boost::detail::function::function_buffer&, boost::python::detail::exception_handler const&, boost::function0<void> const&)
boost::python::handle_exception_impl(boost::function0<void>)
function_call
PyObject_Call
PyInstance_New
barche commented 12 years ago

Aha, thanks for the comments, I'll look into these. The last one seems to indicate that we do need some sort of fallback for getattr, since tab completion seems to use it. I think this also provides a hint on how to make it work: we should probably add our keys for components and options to members, and it should work.

tlmquintino commented 12 years ago

My 2 cents:

  1. As @wdeconinck indicated, I agree to give priority to a component in case of name clash. A warning should be issued. Note also that if you assign to a component you will get a nice error (or you should anyway) so preferring a component will in most cases be pretty obvious to the user that something is wrong.
  2. we should however keep the accessor functions
    model.get_child('NavierStokes')
    model.get_option('NavierStokes')
  1. we could provide a disambiguation that is consistent for components and options (in which case we don't really need the accessor functions):
    model.subcomponents().NavierStokes
    model.options().NavierStokes = on
barche commented 12 years ago

Willem, the test is working now. The problem was that the options were not marked as basic, so they were just added as python attributes. I've changed the behavior in ComponentWrapper to throw an error when a non-existing attribute is set, rather then just let python add it.

barche commented 12 years ago

OK, I've changed the way options behaves. You can now do any of these: model.options().NavierStokes model.options.NavierStokes model.options['NavierStokes'] model.options()['NavierStokes']

The same for child components, but then with children instead of options.