Imports revisited #248

hsorby · 2018-07-17T01:52:40Z

Moving @nickerso work from outside the library to inside. The added import functionality only works for local on disk imports.

Addresses #238.

This PR supersedes #167, should this PR be merged then #167 should be closed and not merged.

…ons_import test model.

Starting work on a test to explore resolving of imports.

Testing that the correct number of imports is detected.

Added a few new methods to the import class to track the source model used to resolve the import. And then the test code which makes use of this to resolve the imports in the existing test model.

This seems to have exposed some issues in the parsing of encapsulation hierarchies, so parking this for now to explore that issue.

The tests are now working as expected (see cellml#171), and all passing.

Models don't all validate successfully, but outstanding issues likely due to cellml#174 and cellml#175.

… in complex_imports.xml.

…rting.

Minor suggestions in documentation

nickerso · 2018-07-21T00:45:41Z

The introduction of the enable_share_from_this is giving me the below warning with VS 2017 every time model.h is included.

3>c:\users\...\libcellml\src\api\libcellml\model.h(36): warning C4251: 'std::enable_shared_from_this<libcellml::Model>::_Wptr': class 'std::weak_ptr<_Ty>' needs to have dll-interface to be used by clients of class 'std::enable_shared_from_this<libcellml::Model>'
3>        with
3>        [
3>            _Ty=libcellml::Model
3>        ]
3>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.14.26428\include\memory(2000): note: see declaration of 'std::weak_ptr<_Ty>'
3>        with
3>        [
3>            _Ty=libcellml::Model
3>        ]

nickerso · 2018-07-21T00:52:42Z

src/model.cpp

+                libcellml::ModelPtr model = parser.parseModel(buffer.str());
+                if (model) {
+                    importSource->setModel(model);
+                    model->resolveImports(url);


Won't this cause all imports in the imported model to be resolved, regardless of whether they are needed?

OK, so we always try to resolve all imports in all imported models, but the hasUnresolvedImports method only checks the required imports are resolved. I guess this is more an efficiency thing that an actual problem with the code.

My point was that, from the code in Parser::parseModel(), model will never be equal to nullptr, so no point for the test.

yep - the if (model) can be removed.

agarny · 2018-07-21T01:06:50Z

The introduction of the enable_share_from_this is giving me the below warning with VS 2017 every time model.h is included.

Could you try to add the following and see whether it makes any difference with VS 2017?

...

#include "libcellml/componententity.h"
#include "libcellml/exportdefinitions.h"

#ifndef SWIG
template class LIBCELLML_EXPORT std::enable_shared_from_this<libcellml::Model>;
#endif

//! Everything in libCellML is in this namespace.
namespace libcellml {

...

…ull/248/files#r204197789.

agarny · 2018-07-21T04:51:50Z

Ok, my bad, I should have read your warning message properly. My idea was correct, except that it should have read:

#ifndef SWIG
template class LIBCELLML_EXPORT std::weak_ptr<libcellml::Model>;
#endif

So, now, everything should be fine (it is using my local copy of VS 2017).

nickerso

Phew! Everything is back working with no warnings for me. I'm happy with the changes Alan has made, but would be good for someone with a bit more C++ expertise to confirm the new approach for import recursion makes sense.

…er to libcellml/types.h.

MichaelClerx · 2018-07-23T09:31:03Z

Happy to look at the bindings bit for this once you're sure the interface is final!

nickerso · 2018-07-31T01:56:40Z

pretty sure the interface is final, so would be good to look at the bindings.

MichaelClerx

Happy to merge this now, from a bindings point of view

agarny · 2018-08-14T14:20:24Z

src/api/libcellml/model.h

+     * @brief Resolve all imports in this model.
+     *
+     * Resolve all @c Component and @c Units imports by loading the models
+     * from local disk through relative urls.  The @p baseFile is used to determine


Might want to replace "urls" with "URLs" to be consistent with the rest of our documentation.

On that note, these methods setUrl and getUrl and such, should they be capitalised this way? Or getURL etc.?
(Personally, my left shoulder likes using the shift key as little as possible)

No, setUrl() and getUrl() should be left as-is. Indeed, we use lower camel case for class methods (see http://libcellml.readthedocs.io/en/latest/dev_coding_standard.html).

agarny · 2018-08-14T14:20:27Z

tests/bindings/python/test_error.py

+
+        # void setReset(const ResetPtr &reset);
+        e = Error()
+        e.setReset(Reset())


Definitely not a blocker, but not sure there is much point in having such a test (and similar ones) since setReset() is effectively tested as part of test_get_reset().

I think you're right, but I just wanted to be systematic in having a test for every method!

agarny · 2018-08-14T14:20:30Z

tests/bindings/python/test_model.py

+        from libcellml import Model
+
+        m = Model()
+        m.resolveImports('file.txt')


No tests as such?

The actual functionality is tested by the C++ tests. Here I just want to check that the method exists and doesn't raise any exceptions!

Fair enough.

agarny · 2018-08-14T14:21:03Z

Apart from a few comments above, it all looks good to me.

David Nickerson and others added 30 commits August 28, 2017 13:20

adding CellML 2.0 versions of the models used by the sine_approximati…

82e4412

…ons_import test model.

Adding inital test code for test import resolution.

cd9d2d0

Starting work on a test to explore resolving of imports.

Hierarchically counting imported components.

1991941

Testing that the correct number of imports is detected.

An initial attempt at resolving imports.

aa50f82

Added a few new methods to the import class to track the source model used to resolve the import. And then the test code which makes use of this to resolve the imports in the existing test model.

Adding more complex import test model.

501e88f

This seems to have exposed some issues in the parsing of encapsulation hierarchies, so parking this for now to explore that issue.

touch up import documentation.

f837f3e

Merging prime develop.

3fd7c84

missed a brace when resolving merge conflicts.

53f5f08

Fixing up component import resolution tests.

ac1c622

The tests are now working as expected (see cellml#171), and all passing.

Correct validation errors.

42111fe

Correcting some errors picked up using the validator.

785fb35

Models don't all validate successfully, but outstanding issues likely due to cellml#174 and cellml#175.

Merge branch 'xml-error-handler' into resolve-imports-tests

d963dd1

Resolve imports updates following merge to prime develop

17a15fe

Merge branch 'xml-error-handler' into resolve-imports-tests

11ea636

Merge branch 'develop' into resolve-imports-tests

32fea1d

Merge in @nickerso's work on resolving imports.

a92469c

Fix further merge conflicts in test resources. Add units to variables…

97f4988

… in complex_imports.xml.

Add the resolution of imported components into the libcellml library.

0701358

Add test resource for model that has an import defined ready for impo…

15476e1

…rting.

Merging in latest changes from configuration_improvements.

eeace86

Merge branch 'develop' into imports_revisited

fd2b981

Merge branch 'develop' into imports_revisited

5b88db8

Mark numerical constants as unsigned in tests.

03bef40

Add importedComponentsCount function.

b08cf2f

Use importedComponentsCount function in imports tests.

b384c61

Minor suggestions in documentation

d18b6d6

Merge branch 'imports_revisited' into hugh_imports

9b309c2

Merge pull request #7 from nickerso/hugh_imports

50a3c0b

Minor suggestions in documentation

Add test for importing units.

6c9a596

Test that the units have successfully been resolved.

11f3976

Model class: explicitly std::enable_shared_from_this<Model>.

d2dfc7d

nickerso reviewed Jul 21, 2018

View reviewed changes

agarny added 2 commits July 20, 2018 15:31

Removed an unneeded test as per https://github.com/cellml/libcellml/p…

2e03d67

…ull/248/files#r204197789.

Windows: prevents a warning from Visual Studio.

97e9509

nickerso approved these changes Jul 21, 2018

View reviewed changes

agarny added 2 commits July 21, 2018 00:18

Slight reordering of our CellML-related type definitions.

b20a2e8

model.cpp: moved our type definition for shared imported entity point…

4a6788a

…er to libcellml/types.h.

MichaelClerx added 8 commits August 14, 2018 13:13

Added test for missing docstrings in bindings.

2c306e1

Resolved several python docstring issues.

8b52dcd

Added missing docstrings.

683a5d1

Added tests for ImportSource bindings.

e66a8ad

Added extra tests for Error bindings.

51f14ee

Added extra tests for Model bindings.

8ea05a4

Adding missing binding tests for ComponentEntity and Printer.

1055ede

Added missing Python binding test.

b947984

MichaelClerx self-requested a review August 14, 2018 13:57

MichaelClerx approved these changes Aug 14, 2018

View reviewed changes

agarny reviewed Aug 14, 2018

View reviewed changes

fix documentation typo.

1744188

nickerso merged commit 41d06d9 into cellml:develop Aug 15, 2018

hsorby mentioned this pull request Aug 16, 2018

setImport and getImport. #188

Closed

nickerso mentioned this pull request May 4, 2019

Validation should check for recursive imports #280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imports revisited #248

Imports revisited #248

hsorby commented Jul 17, 2018

nickerso commented Jul 21, 2018

nickerso Jul 21, 2018

nickerso Jul 21, 2018

agarny Jul 21, 2018

nickerso Jul 21, 2018

agarny commented Jul 21, 2018

agarny commented Jul 21, 2018 •

edited

Loading

nickerso left a comment

MichaelClerx commented Jul 23, 2018

nickerso commented Jul 31, 2018

MichaelClerx left a comment

agarny Aug 14, 2018

MichaelClerx Aug 14, 2018

agarny Aug 14, 2018

agarny Aug 14, 2018

MichaelClerx Aug 14, 2018

agarny Aug 14, 2018

MichaelClerx Aug 14, 2018

agarny Aug 14, 2018

agarny commented Aug 14, 2018

Imports revisited #248

Imports revisited #248

Conversation

hsorby commented Jul 17, 2018

nickerso commented Jul 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarny commented Jul 21, 2018

agarny commented Jul 21, 2018 • edited Loading

nickerso left a comment

Choose a reason for hiding this comment

MichaelClerx commented Jul 23, 2018

nickerso commented Jul 31, 2018

MichaelClerx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarny commented Aug 14, 2018

agarny commented Jul 21, 2018 •

edited

Loading