FGFDMExec.get_property_catalog() is supposed to take 0 arguments, but is says it needs exactly one

[LuFlo]

I'm submitting a ...

• bug report
• feature request
• support request => Please do not submit support request here, see note at the top of this template.

Describe the issue

Given following python code:

import jsbsim

fdm = jsbsim.FGFDMExec(None)
fdm.load_model('c172x')
print(fdm.get_property_catalog())

According to the C++ documentation, getPropertyDialog does not require an argument (it also wouldn't make much sense), see: https://jsbsim-team.github.io/jsbsim/classJSBSim_1_1FGFDMExec.html#ac19c4ddfeb51d183eb9e40d9a8200850

What is the current behavior?

Traceback (most recent call last):
  File "/home/lflo/jsbsim_testing/jsbsim_testing/property_catalog_bug.py", line 5, in <module>
    print(fdm.get_property_catalog())
TypeError: FGFDMExec.get_property_catalog() takes exactly one argument (0 given)

What is the expected behavior?

Just give a list of the properties available (probably)

What is the motivation / use case for changing the behavior?
I would like to print a list of all accelerations available, without hardcoding them

Please tell us about your environment:

• PopOS 22.04
• JSBSim Flight Dynamics Model v1.1.11 [GitHub build 741/commit 10e077b] Feb 13 2022 12:40:33 [JSBSim-ML v2.0]
• Python

Other information

[seanmcleod]

jsbsim/python/jsbsim.pyx.in

Lines 771 to 785 in 59389f6

	def query_property_catalog(self, check):
	"""@Dox(JSBSim::FGFDMExec::QueryPropertyCatalog)"""
	catalog = (self.thisptr.QueryPropertyCatalog(check.encode())).decode('utf-8').rstrip().split('\n')
	if len(catalog) == 1 and catalog[0] == "No matches found":
	return []
	else:
	return catalog
	def get_property_catalog(self, check):
	"""Retrieves the property catalog as a dictionary."""
	catalog = {}
	for item in self.query_property_catalog(check):
	property_name = item.split(" ")[0] # remove any (RW) flags
	catalog[property_name] = self.get_property_value(property_name)
	return catalog

jsbsim/src/FGFDMExec.h

Lines 512 to 518 in 59389f6

	/** Retrieves property or properties matching the supplied string.
	* A string is returned that contains a carriage return delimited list of all
	* strings in the property catalog that matches the supplied check string.
	* @param check The string to search for in the property catalog.
	* @return the carriage-return-delimited string containing all matching strings
	* in the catalog. */
	std::string QueryPropertyCatalog(const std::string& check);

So it takes a filter argument, if you don't want to filter.

>>> fdm.get_property_catalog('')

{'inertial/sea-level-radius_ft': 20925646.32546, 'simulation/gravity-model': 1.0, 'simulation/integrator/rate/rotational': 1.0, 'simulation/integrator/rate/translational': 3.0, 'simulation/integrator/position/rotational': 1.0, 'simulation/integrator/position/translational': 4.0, 

[LuFlo]

I see, jsbsim.pyx.in is a good hint, thanks for that! So there are some differences with the original C++ function, or is the C++ function subject to change?

jsbsim/src/FGFDMExec.h

Line 526 in 59389f6

std::vector<std::string>& GetPropertyCatalog(void) {return PropertyCatalog;}

I still have some issues with properties, since I am not able to reset the accelerations properly (the aircraft always starts with some acceleration and consequently with velocities), but I will open a new discussion thread for this as soon as I've done some more digging.

[seanmcleod]

So there are some differences with the original C++ function

Yes there are a couple of differences, i.e. the python version takes a filter option and returns a dictionary, whereas the C++ version doesn't take the filter option and returns a vector/array rather than a dictionary.

or is the C++ function subject to change?

JSBSim is open source and doesn't make any API guarantees so yes it could change.

[bcoconni]

or is the C++ function subject to change?

JSBSim is open source and doesn't make any API guarantees so yes it could change.

Well, indeed the current Python API for FGFDMExec.get_property_catalog() was introduced in the commit that introduced the Python module and indeed I see no reason why the Python API should be different than the C++ API ?

[seanmcleod]

The C++ API looks a bit messy/inconsistent to me.

  std::vector<std::string> PropertyCatalog;

  std::vector<std::string>& GetPropertyCatalog(void) {return PropertyCatalog;}

  string FGFDMExec::QueryPropertyCatalog(const string& in)
  {
    string results;
    for (auto &catalogElm: PropertyCatalog) {
      if (catalogElm.find(in) != string::npos) results += catalogElm + "\n";
    }
    if (results.empty()) return "No matches found\n";
    return results;
  }

At a high-level the one GetPropertyCatalog method is a special case of QueryPropertyCatalog simply with the in filter being blank/match any/all.

But the one returns a vector of strings and the other a single string delimited by \n, as if it's designed specifically for human output.

So how about consolidating them into a single method?

  std::vector<std::string> PropertyCatalog;

  std::vector<std::string>& FGFDMExec::QueryPropertyCatalog(const string& in = "")
  {
    if(in.length() < 1)
      return PropertyCatalog;

    std::vector<std::string> results;
    for (auto &catalogElm: PropertyCatalog) {
      if (catalogElm.find(in) != string::npos) results.push_back(catalogElm);
    }
    return results;
  }

[seanmcleod]

The the other difference with the current python get_property_catalog method is that unlike the C++ API it returns the current value for each property as well as opposed to just the property name.

    def get_property_catalog(self, check):
        """
        Retrieves the property catalog as a dictionary.
        """
        catalog = {}
        for item in self.query_property_catalog(check):
            catalog[item] = self.get_property_value(item)
        return catalog

[LuFlo]

Well, indeed the current Python API for FGFDMExec.get_property_catalog() was introduced in the commit that introduced the Python module and indeed I see no reason why the Python API should be different than the C++ API ?

I actually don't mind the difference, but in the original doxygen doc it should say that the python API may differ, or the Readme page should reference the .pyx.in file as proper documentation.

or is the C++ function subject to change?

JSBSim is open source and doesn't make any API guarantees so yes it could change.

Well yeah, obviously there is no guarantee for anything, in fact private / proprietary software also don't necessarily provide API guarantees. But especially open software is included in many other projects, so why introduce breaking changes? I was just asking, if there is a plan specific for these kind of functions to output maps rather than vectors.

I also don't mind if there are features lacking or if the python module is incomplete. But the Readme states: "A Python module which provides the exact same features than the C++ library". That's why I thought that maybe I've done something wrong or that there may be some bugs. I just now am beginning to dig deeper, for example I realized that the FGPropagate class has almost none of the original accessors. So maybe we should include a little disclaimer that the python module still needs some work. What do you think?

[bcoconni]

@seanmcleod as the C++ API largely predates the Python module, I am not too keen on modifying the C++ API even if, as you mentioned, it is a bit messy and inconsistent. I'd rather update the Python API.

@LuFlo, as you mentioned, we are advertising that the Python module provides the exact same features than the C++ library. Sure we could argue that providing the same features and providing the same API are somewhat different and we could update the docs stating that FGFDMExec.get_property_catalog and FGFDMExec.query_property_catalog are slightly different than their C++ counterpart as they provide a couple of additional features. Still, users will fall in the same trap than you did because it is only natural to expect that the Python API is identical to the C++ API.

My vote would rather be to update the Python code of FGFDMExec.get_property_catalog and FGFDMExec.query_property_catalog to behave identically to their C++ counterparts.

[agodemar]

@bcoconni @seanmcleod
My vote is to have the Python code of FGFDMExec.get_property_catalog and FGFDMExec.query_property_catalog behave identically to their C++ counterparts. And to have a distint Python function that filters the whole catalog, named for instance FGFDMExec.find_property_catalog accepting a string. As a user, I'd like to have the same behaviour also in the C++ API.

[seanmcleod]

I guess I'm out voted 2-1 😉 Changing only the python API will only affect python users as opposed to my suggestion which would affect both C++ users and python users.

Python code... behave identically to their C++ counterparts.
And to have a distinct Python function that filters the whole catalog,

@agodemar I'm a bit confused because the existing C++ API already has a function that filters the catalog, i.e. QueryPropertyCatalog, so there is no need for a distinct Python function if the Python API is made identical to the C++ API.

  std::vector<std::string> PropertyCatalog;

  std::vector<std::string>& GetPropertyCatalog(void) {return PropertyCatalog;}

  string FGFDMExec::QueryPropertyCatalog(const string& in)
  {
    string results;
    for (auto &catalogElm: PropertyCatalog) {
      if (catalogElm.find(in) != string::npos) results += catalogElm + "\n";
    }
    if (results.empty()) return "No matches found\n";
    return results;
  }

[agodemar]

ok @seanmcleod so we are almost done ;-) My fault, didn't recall QueryPropertyCatalog's signature

[LuFlo]

@agodemar I'm a bit confused because the existing C++ API already has a function that filters the catalog, i.e. QueryPropertyCatalog, so there is no need for a distinct Python function if the Python API is made identical to the C++ API.

There still seems to be some differences. For example, the python version outputs a list, instead of the string, also the python version has the R / RW indicators, unfortunately I can't test the C++ version yet, because I only have access to my mobile setup atm.

I would suggest to make the check variable of get_property_dialog optional like this:

def get_property_catalog(self, check=''):
    """Retrieves the property catalog as a dictionary."""
    catalog = {}
    for item in self.query_property_catalog(check):
        property_name = item.split(" ")[0]  # remove any (RW) flags
        catalog[property_name] = self.get_property_value(property_name)
    return catalog

As far as I'm aware Cython should support optional keyword arguments. That way existing API-uses don't brake and you still have the flexibility.

The trickier question is if we should also change the query_property_catalog signature, since the C++ version is indeed somewhat strange :D

[seanmcleod]

@LuFlo both @bcoconni and @agodemar mentioned making the Python API identical to the current C++ API.

to behave identically to their C++ counterparts.

[LuFlo]

Oh yeah, then outputting a list instead of a dictionary of course, sorry for the confusion.

[bcoconni]

This issue should now be fixed by the commit 157f91e.
Please check the code from branch master and let us know if the bug is indeed fixed following your tests.

[bcoconni]

The problem reported in this issue is now considered fixed and the issue is closed.