-
Notifications
You must be signed in to change notification settings - Fork 47
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
Visualize: fix flatten of observable mapping with one observable #1515
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1515 +/- ##
===========================================
+ Coverage 82.76% 84.32% +1.55%
===========================================
Files 163 163
Lines 14016 14013 -3
===========================================
+ Hits 11601 11816 +215
+ Misses 2415 2197 -218 ☔ View full report in Codecov by Sentry. |
@Doresic Looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add a test for future cases probably
I think there are two more such cases in |
test/visualize/test_visualize.py
Outdated
# Set seed for reproducibility. | ||
np.random.seed(0) | ||
optimizer = pypesto.optimize.ScipyOptimizer( | ||
method="L-BFGS-B", options={"maxiter": 10} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For just testing that visualization does not produce an error, a single iteration would suffice, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. From my side this can be merged.
Observable mapping visualization failed in the case of only one observable. Fixed to account for the case of one observable.