Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 334 - local inputs connected to promoted variables #341

Merged
merged 4 commits into from
May 21, 2021

Conversation

ScottDelbecq
Copy link
Contributor

This PR fixes #334 (partly solved by #332) where some variables are tagged as inputs instead of outputs when using VariableList.from_problem().

To fix this, VariableList.from_problem() was modified to handle the effect of using connect() with local inputs and promoted outputs. This behaviour can be observed when using list_variables() or the variable_viewer().

However, generate_inputs() works fine and gives the correct input list because it uses _utils.get_unconnected_input_names(). This brings up the fact that VariableList.from_problem() is too complicated for what we really need it to do and should deserve a rework in a later PR.

Copy link
Contributor

@christophe-david christophe-david left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works Ok, thanks.

A remark on the order of commits: it is better to implement the test, then the fix. It makes easier for the reviewer to checkout the commit with the test, see that it fails, then checkout the head of the branch and see that the test passes.
Even if you did not work in this order (though it is generally advised to do so), an interactive rebase allows to put your commit history in the right order.

I just ask for a change in a comment and it will be Ok.

src/fastoad/openmdao/variables.py Outdated Show resolved Hide resolved
src/fastoad/openmdao/variables.py Show resolved Hide resolved
@ScottDelbecq
Copy link
Contributor Author

Thanks for you comments. Can we integrate this PR in #340?

@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #341 (fbe8bbb) into master (682f212) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   91.09%   91.11%   +0.02%     
==========================================
  Files         191      191              
  Lines        7648     7655       +7     
  Branches      726      730       +4     
==========================================
+ Hits         6967     6975       +8     
+ Misses        546      542       -4     
- Partials      135      138       +3     
Impacted Files Coverage Δ
src/fastoad/openmdao/variables.py 91.16% <100.00%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 682f212...fbe8bbb. Read the comment docs.

Copy link
Contributor

@christophe-david christophe-david left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes.
Sorry, in the previous review, I missed you commented out some code (that, indeed, should legitimately be removed).

src/fastoad/openmdao/variables.py Outdated Show resolved Hide resolved
@christophe-david christophe-david merged commit 7f35925 into master May 21, 2021
@christophe-david christophe-david deleted the issue-334_global_inputs branch May 21, 2021 09:52
@christophe-david
Copy link
Contributor

The change has been integrated in #340.
Looking forward your review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connected promoted inputs are considered as global inputs
2 participants