A number of links in the Deviser Manual still refer to Sourceforge. These need updating (assuming we no longer want to use Sourceforge)

Deviser output writes tex files (particularly the validation appendix) and hard-wraps the lines at 80 characters. This makes it difficult to run search-and-replace functions, which are particularly necessary when dealing with non-SBML specifications, but can also be a pain for normal SBML package specifications. If hard wrapping was removed, it would make dealing with these files easier.

I have added the tests originally in run_cpp_tests.py to generator/pytest_files/test_cpp/test_cpp.py. Generally things are working well, but we need to make sure silly input causes a test failure, which it currently doesn't. e.g. using Defggggggggg as a class name.

In places these appear to be the same

In XML schema document deviser/docs/deviser.rng, some content is missing.

Currently there is no information for <language> nodes and their children, e.g. as in samples/sbgn.xml.

Once this is corrected, we ought to write a little script to iterate over the samples and see if they are fully covered by the schema. Although that assumes the samples cover every type of node that Deviser might encounter, and I don't know if that is the case.

When generating LaTeX validation rules, the internal ID is used instead of the 'XML name'.

As an example:

The 'rangeId' attribute is actually just 'range' in the XML, but the validation rules (both in the generated C++ and in the LaTeX) claim that it is 'rangeId'.

It would seem that the getAllElements functions are not being generated. It would also be nice to have the ElementFilter functionality as we have it in libSBML automatically generated.

The read_language_element functions reads seeveral XML elements. It would be cleaner split into several functions

In deviser/parseXML/ParseXML.py, there is a function to_bool(). It is not clear what is/should be returned if v.lower() is "false".

Several of these functions do not return anything which is a little counter-intuitive for a function called 'get'

None of my tests actual return true from this function.

In ParseXML.py, function get_loclass_name_value(), comment says "we expect this to be camelCase starting with lower", but code returns a string starting with upper case.

In util/generateCode.py, function create_dir_structure() takes a parameter overwrite. If this param is False, the function executes the line print('directory already exists - terminating to avoid overwriting'). BUT just before that, there is a call to create_dir(directories[index], overwrite)

If I look at create_dir(), it has the signature:
def create_dir(name, skip_existing):

So the skip_existing parameter is the same as overwrite. But surely these want different outcomes to each other? Or have I missed something??

E.g. if overwrite is True (I want to overwrite any existing directory with this name), this means skip_existing will be True. So, if the directory already exists, the create_dir() function just skips it and says done = True … but it isn’t?!

In ParseXML.py function parse_deviser.xml(), the following lines appear:

    sbml_level = 3
    sbml_version = 1
    pkg_version = 1

We need to discuss what to do about these numbers. e.g. should they be stored in a "versions" file which Deviser reads in?

The returns of these functions in query.py are not intuitive

• get_element_name
• prefix_classes
• prefix_classes

I've noticed that when using two libraries generated by deviser, some enum names are unfortunately, causing compile errors (for example using latest combine archive / sedml, if included together). Most names are already prefixed, but some are not (like AllowedAttributes).

This probably needs to be more generic

In directory deviser/generator/util, there are a number of files which have almost identical structures:

generateCode.py, generateExamples.py, generateLatex.py, generateRNG.py, generateTest.py and generateXML.py.

They can all be invoked from the command line and:

• check the number of command-line arguments is correct
• Invoke ParseXML.ParseXML()on the filename supplied at the command line
• generate big dictionary object structure from ParseXML.parse_deviser_xml()
• then invoke creation of the desired file type using the big structure object just obtained.

I think there is a lot of scope for merging these into one file, with command-line options to enable selection of the desired output file type.

NB Obviously any client code needs to be identified and updated, too.

Up until L3V2 a ListOf element in SBML could not be empty so the min no of children in a listOf defaults to 1. L3V2 changed this and also other standards do not have this rule.

Need to check

• get_lo_min_children() and its usage

• minNumListOfChildren variable

In deviser/generator/tests/test_cpp_code, run_cpp_tests.py experiences two test failures (for both Python 2 and Python 3):

REPORT for CPP Tests
All tests were run.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
2 fails reported:
test-code/Def.h
test-code/Def.cpp

In deviser/util/strFunctions.py, function singular() correctly deals with some plurals ending in "es" but not others.

e.g. singular("foxes") -> "fox". ✔️
but
singular("apples") -> "appl" 👎

In the Deviser README file, some links still refer to Sourceforge.

Presumably these need updating to Github links?

Matthew tidied this up and commented out some code for Sarah to look at.

If I set an attribute as being a 'positive int', the resulting validation rule says that it must be non-negative. This is incorrect. Non-negative means [0, 1, ...] and positive means [1, 2, ...].

David suggests we add code to make deviser an installable package.

Instructions page.

And add appropriate entry point(s).

This article may be useful as well. It mentions a lot of code quality tools.

I've been pulling various SBML-adjacent projects into a 'dependencies' project that builds everything at once for the various things we make. If I put more than one Deviser-generated project in there, I get CMake errors that there are multiple 'dist' targets. The dist target was a holdover from gnumake days; it's probably best to drop it.

Generation for 2 depends

As discussed in this issue: sbmlteam/libsbml#36, the current libSBML only compiles with c++ standard up to 14. This is because of the use of unary_function and ptr_function. The code compiles fine without these, so all that needs doing is not to inherit from unary functions.

Deviser generates them for finding elements by id or reference:

 /**
  * Used by ListOf::get() to lookup an SBase based by its id.
  */
 struct IdEqS : public unary_function<SBase*, bool>
 {
   const string& mId;
 
   IdEqS (const string& id) : mId(id) { }
   bool operator() (SBase* sb) 
        { return static_cast <Species *> (sb)->getId() == mId; }
 };

this struct will work just as well, by not using the unary_function.

It is too close to base_files/BaseCMakeFile.py
It isn't strictly speaking a base class

Currently we are being funded by the Chan Zuckerberg Initiative, and work is being done by paid employees at UCL.

We should add CZI (and UCL?) acknowledgements or whatever in the files such as init.py etc

The writing tex command fails with later python versions need to fix and then address documenting strfunctions

• wrap_enum
• wrap_section
• warp_token
• wrap_type

This was unintuitive

see run_other_library_tests
generate_enum

It seems to me it would fit more naturally there.

This is the big function in generator/base_files/BaseFile.py.

Ideally it needs refactoring/splitting into functions, and new tests.

In file deviser/generator/util/global_variables.py, there are a lot of lines like:

global language
language = 'sbml'

and, further down the file:

def set_globals(lang, ...):
    global language
    language = lang
    ...

There are an awful lot of global declarations, seemingly to allow the varables so-called to be accessed in the functions in the lower part of the file.

We could consider replacing the code in this file with a new GlobalVariables class. That would make the code simpler. The set_globals() function would be replaced by a constructor. Then pairs of lines such as those shown in the set_globals() function shown above would be replaced with one line, e.g.:

def __init(lang,...):
    self.language = lang
    ....

Already, in client code, we see things like:

import global_variables as gv
...
    if gv.code_returned == gv.return_codes['success']:
        print('code successfully written')
...

This could be replaced by something like:

import GlobalVariables as GV
...
   if GV.code_returned == ...

I'm guessing we'd probably need static functions so we don't require an object instance, in which case the GlobalVariables constructor could be replaced with a suitable @staticmethod etc...

What do people think about this suggestion?

sonar noticed that the code generated for removeChildObject is invalid. What happens is that

• a ptr to the object is retained
• then the object is freed
• then the ptr to the freed element is returned for reuse, which ought to be fixed.

Instead of a call to unsetXXX the elements member variable should just be set to NULL.

In util/generateCode.py, function create_dir_structure() takes a parameter overwrite. If this param is False, the function executes the line print('directory already exists - terminating to avoid overwriting'). BUT just before that, there is a call to create_dir(directories[index], overwrite)

If I look at create_dir(), it has the signature:
def create_dir(name, skip_existing):

So the skip_existing parameter is the same as overwrite. But surely these want different outcomes to each other? Or have I missed something??

E.g. if overwrite is True (I want to overwrite any existing directory with this name), this means skip_existing will be True. So, if the directory already exists, the create_dir() function just skips it and says done = True … but it isn’t?!

Mark items necessary for generating an 'other' library depending on either libsbml or liblx C++

• to_int
• get_enum_value
• get_enum_name
• find_lo_element

This function does two different things and has two different returns.
Needs review

I just downloaded Deviser Installer 1.0.0 loaded the groups.xml sample and in the generate menu it produced "Unknown Error Please Report"

Currently, for Distrib, the following function is generated:

void
UncertParameter::renameSIdRefs(const std::string& oldid,
                               const std::string& newid)
{
  if (isSetVar() && mVar == oldid)
  {
    setVar(newid);
  }

  if (isSetUnits() && mUnits == oldid)
  {
    setUnits(newid);
  }
}

However, this is incorrect: the units are not SIdRefs, and are instead UnitSIdRefs. There is a completely different function for renaming units, named renameUnitSIdRefs, which instead needs the unit renaming code.

In addition, UncertParameters have MathML children, which may have both SIdRefs and UnitSIdRefs, in them. The upshot is that instead of the above single function, we instead need the following:

void
UncertParameter::renameSIdRefs(const std::string& oldid,
                               const std::string& newid)
{
  DistribBase::renameSIdRefs(oldid, newid);
  if (isSetVar() && mVar == oldid)
  {
    setVar(newid);
  }

  if (isSetMath()) 
  {
      mMath->renameSIdRefs(oldid, newid);
  }
}

void
UncertParameter::renameUnitSIdRefs(const std::string& oldid, const std::string& newid)
{
    DistribBase::renameUnitSIdRefs(oldid, newid);

    if (isSetUnits() && mUnits == oldid)
    {
        setUnits(newid);
    }

    if (isSetMath()) 
    {
        mMath->renameUnitSIdRefs(oldid, newid);
    }
}

Also, there is one more function that needs to be implemented:

/** @cond doxygenLibsbmlInternal */
void
UncertParameter::replaceSIDWithFunction(const std::string& id, const ASTNode* function)
{
    if (isSetMath()) {
        if (mMath->getType() == AST_NAME && mMath->getName() == id) {
            delete mMath;
            mMath = function->deepCopy();
        }
        else {
            mMath->replaceIDWithFunction(id, function);
        }
    }
}
/** @endcond */

This function is for handling the cases where the replacement needs to be scaled by some conversion factor.

Unfortunately, there's no already-built-in way to convert the value of any UncertParameter by a conversion factor, but hopefully that won't become an issue. The units will still be correct, if set.

Also, the UncertSpan::renameSIdRefs() needs to call the base class:

void
UncertSpan::renameSIdRefs(const std::string& oldid, const std::string& newid)
{
  UncertParameter::renameSIdRefs(oldid, newid);
  if (isSetVarLower() && mVarLower == oldid)
  [...]

(In general, the rename functions should always call the base class.)

TODO will this throw an exception if elements other
than the first one do not have the attribute?
Is that possible?
"

I've noticed that all deviser generated writers have issues with dereferencing NULL documents when writing. (This also happens in libSBML). The fix needs to go to:

generator/code_files/templates/Writer.cpp

This also needs to check that the error log is not NULL.

In directory deviser/generator/tests/test_other_library, the file run_other_library_tests.py is having a number of failures. This is due to differences between the reference files and the test/temp files.

I have only looked at a couple of these, but it appears that the test/temp files have additional code which makes them more robust. I therefore suspect that (some of) the reference files are out-of-date.

The branch I was working on for this was matt-tests

The failure cases are:

temp/sedml/SedModel.cpp
temp/sedml/SedListOfModels.h
temp/sedml/SedListOfModels.cpp
temp/sedml/SedDocument.cpp
temp/sedml/SedSetValue.cpp
temp/sedml/SedDataGenerator.cpp
temp/sedml/SedRepeatedTask.cpp
temp/sedml/SedSimulation.cpp
temp/sedml/SedAbstractTask.cpp
temp/sedml/SedListOfTasks.h
temp/sedml/SedListOfTasks.cpp
temp/sedml/SedOutput.cpp
temp/sedml/SedListOfOutputs.h
temp/sedml/SedListOfOutputs.cpp
temp/sedml/cmake/src/CMakeLists.txt
temp/sedml/cmake/src/bindings/CMakeLists.txt
temp/sedml/cmake/src/sedml/CMakeLists.txt
temp/omex/CaContent.cpp
temp/omex/CaListOfContents.h
temp/omex/CaListOfContents.cpp
temp/omex/cmake/src/CMakeLists.txt
temp/omex/cmake/src/bindings/CMakeLists.txt
temp/omex/cmake/src/omex/CMakeLists.txt

It should be borne in mind that the problems probably lie in the corresponding reference files, not the above test files.

the xml description has nested elements; documentation needs to be clear