Creation of the LEGOProblem class #13

imcovangent · 2018-08-06T10:50:32Z

This pull request introduces a new class called LEGOProblem. This is a subclass of the OpenMDAO Problem class which:

provides the ability to instantiate a Problem object based on a CMDOWS file with fully configured LEGOModel, converger groups and drivers for the OpenMDAO object.
includes some helpful utilities to automatically configure a case reader and read out (and optionally print out) its main results in a dictionary object based on the type of run that was executed (MDA, DOE, MDO).

All test case in the OpenLEGO test suite have also been updated and validated using the new LEGOProblem class.

…st all KADMOS architectures (MDA, DOE, monolithic MDO) are now supported. Included some new utility functions.

Added documentation to problem.py file. Added automatic case reader and printing functionality. Added many new CMDOWS test files for the test suite. Updated sellar_competences_test, still to do other tests.

# Conflicts: # openlego/test_suite/test_examples/sellar_competences/sellar_competences_test.py

Changed print_results() method to collect_results() method with optional printing and return of results dictionary. Added utility function to print with Boolean (print_optional(string, print_in_log)), alternative would be to use a logger. Added utility function to clean a directory based on a filter (clean_dir_filtered(dir, filters)) Added utility function to expand a dictionary safely (add_or_append_dict_entry(main_dict, main_key, sub_key, value)

Small updates to other test files.

daniel-de-vries

Overall: really great work! There are a few small things I want to address. Please have a look.

daniel-de-vries · 2018-08-06T15:20:39Z

openlego/core/problem.py

+class LEGOProblem(Problem):
+    """Specialized OpenMDAO Problem class representing the problem specified by a CMDOWS file.
+
+    # TODO: Daniel: is this also true here?


Yes, this is also true here.

Ok, then we keep the comment.

daniel-de-vries · 2018-08-06T15:21:05Z

openlego/core/problem.py

+    An important note about this class in the context of OpenMDAO is that the aggregation pattern of the root Problem
+    class has is changed into a stronger composition pattern. This is because this class directly
+    controls the creation and assembly of this class by making use of Python's @property decorator. It is not possible,
+    nor should it be attempted, to manually inject a different instance of Problem in place of these, because the


these -> this

daniel-de-vries · 2018-08-06T15:22:15Z

openlego/core/problem.py

+                print_optional('Optimum found!', print_in_log)
+            else:
+                print_optional('Driver finished!', print_in_log)
+        print_optional('\nPrinting case numbers: {}'.format(num_cases, cases_to_collect), print_in_log)


Too many arguments for the string format.

Well spotted! :)

daniel-de-vries · 2018-08-06T15:28:00Z

openlego/core/problem.py

+    def driver_type(self):
+        # type: () -> Union[str, None]
+        """:obj:`str`: Type of driver as string."""
+        assert len(self.drivers['optimizers']) + len(self.drivers['does']) <= 1, \


Codacy is used on this repo to help ensure good code quality. One of the flags it threw was that it is better to use a construction like

if condition: raise AssertionError('message')

instead of assert condition, 'message', because the protections these assertions offer are removed when the code is compiled. Furthermore, it is considered general bad practice. So please change this and the other assertions to a if-then construction.

Note that this does not apply to the assertion methods used in TestCases. There it is warranted by the unittest framework.

Ok, all assert code lines have been adjusted as suggested.

daniel-de-vries · 2018-08-06T15:28:57Z

openlego/core/problem.py

+            results : dict of dicts
+                Dictionary containing the results that were collected
+        """
+        assert os.path.isfile(self.case_reader_path), 'Could not find the case reader file {}.'\


See previous comment about assertions

Adjusted as suggested.

daniel-de-vries · 2018-08-06T15:29:08Z

openlego/core/problem.py

+            .format(self.case_reader_path)
+        assert isinstance(cases_to_collect, (str, list)), 'cases_to_print must be of type str or list.'
+        if isinstance(cases_to_collect, str):
+            assert cases_to_collect in ['default', 'all', 'last'], 'Invalid cases_to_print string value provided.'


See previous comment about assertions

Adjusted as suggested.

daniel-de-vries · 2018-08-06T15:31:58Z

openlego/utils/cmdows_utils.py

@@ -103,6 +106,37 @@ def get_uid_search_xpath(uid):
        return './/*[@uID="' + uid + '"]'


+def get_opt_setting_safe(opt_elem, setting, default, expected_type='str'):
+    # TODO: Add docstring and type


Please indeed add the docstring and type hint

daniel-de-vries · 2018-08-06T15:32:25Z

openlego/utils/cmdows_utils.py

+
+
+def get_doe_setting_safe(doe_elem, setting, default, expected_type='str', doe_method=None, required_for_doe_methods=None):
+    # TODO: Add docstring and type


Please indeed add the docstring and type hint

daniel-de-vries · 2018-08-06T15:33:49Z

openlego/utils/general_utils.py

+        raise IOError('expected_type "{}" is not supported in this function.'.format(new_type))
+
+
+def clean_dir_filtered(dir, filters):


dir is a built-in parameter in Python. Shadowing these is considered bad practice, so please change this local variable name.

Changed to dr (also file has been changed to f).

daniel-de-vries · 2018-08-06T15:34:08Z

openlego/utils/general_utils.py

+    # type: (path, List[str]) -> None
+    """Removes files in a directory that contain the strings provided in the filter."""
+    for file in os.listdir(dir):
+        for filter in filters:


filter is a built-in parameter in Python. Shadowing these is considered bad practice, so please change this local variable name.

Changed to fltr

imcovangent · 2018-08-07T09:58:18Z

All requested changes have been addressed. Ready to pull, I hope! :)

imcovangent added 9 commits August 2, 2018 11:33

First version of new class LEGOProblem(Problem) and testing of new fork.

871c3e5

Finished and tested new LEGOProblem class for Sellar test cases. Almo…

cf0005a

…st all KADMOS architectures (MDA, DOE, monolithic MDO) are now supported. Included some new utility functions.

Finished and tested LEGOProblem class on all Sellar and SSBJ test cases.

b321a99

Added documentation to problem.py file. Added automatic case reader and printing functionality. Added many new CMDOWS test files for the test suite. Updated sellar_competences_test, still to do other tests.

Updated test with Sellar competences.

a24e877

Merge branch 'master' of https://github.com/daniel-de-vries/OpenLEGO

182f2f6

# Conflicts: # openlego/test_suite/test_examples/sellar_competences/sellar_competences_test.py

Updated SSBJ test file.

dd0bc92

Small updates to other test files.

Small documentation updates.

cf04823

Small type hinting updates

6214063

daniel-de-vries requested changes Aug 6, 2018

View reviewed changes

imcovangent added 3 commits August 7, 2018 11:53

Code adjustments based on pull-request feedback.

2355a0d

Code adjustments based on pull-request feedback.

321a338

Reverted model.py back to end of phase 2 developments.

fe4b229

daniel-de-vries approved these changes Aug 7, 2018

View reviewed changes

daniel-de-vries merged commit ac9c615 into daniel-de-vries:master Aug 7, 2018

This was referenced Aug 7, 2018

Add driver and other problem characteristics to OpenLEGO #8

Closed

Add all monolithic options from KADMOS to OpenLEGO #6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creation of the LEGOProblem class #13

Creation of the LEGOProblem class #13

imcovangent commented Aug 6, 2018

daniel-de-vries left a comment

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

daniel-de-vries Aug 6, 2018

imcovangent Aug 7, 2018

imcovangent commented Aug 7, 2018



		def get_doe_setting_safe(doe_elem, setting, default, expected_type='str', doe_method=None, required_for_doe_methods=None):
		# TODO: Add docstring and type

		raise IOError('expected_type "{}" is not supported in this function.'.format(new_type))


		def clean_dir_filtered(dir, filters):

Creation of the LEGOProblem class #13

Creation of the LEGOProblem class #13

Conversation

imcovangent commented Aug 6, 2018

daniel-de-vries 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imcovangent commented Aug 7, 2018