ibpsa / project1-boptest

Building Optimization Performance Tests
Other
102 stars 66 forks source link

Concerns Regarding the Fix for Issue #537 #565

Closed Enderdead closed 10 months ago

Enderdead commented 10 months ago

Hello,

I've carefully reviewed the fix implemented for Issue #537 and would like to raise a few concerns about its approach. Indeed, the 3 examples python file in its current state cannot be executed due to import error as it is shown in the issue #537 . The conclusion of this issue is to ask the user to add the project root to pythonpath on his own.

However, I noticed that the original author had already attempted to address this with a PYTHONPATH update but made an error in the sequence of the imports: https://github.com/ibpsa/project1-boptest/blob/db0f10ef3af881f035b8a5f574dc24987ebd4916/examples/python/testcase1.py#L13-L16

On line 15, the root directory is added to the PYTHONPATH, which should support the import on line 14. But since these lines are in the wrong order, it results in an error.

The solution to Issue #537 could simply be to swap these two lines. I would like to get your feedback on this observation before I proceed to create a PR.

Have a great day!

dhblum commented 10 months ago

Thanks @Enderdead for reporting. Let's fix what you report in project1-boptest/examples/python/testcase1.py (as well as /testcase2.py and /testcase3.py where I see the same issue). But I suggest also keeping the added line to the README about needing to add to PYTHONPATH, though maybe add a note that this is done in the example scripts already, since it is useful to know this is happening when someone runs the example controllers.

dhblum commented 10 months ago

Care to propose a PR?

dhblum commented 10 months ago

Closed by https://github.com/ibpsa/project1-boptest/pull/575.