-
-
Notifications
You must be signed in to change notification settings - Fork 409
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
Fix for widgets displaying code in commandline #2253
Fix for widgets displaying code in commandline #2253
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2253 +/- ##
==========================================
- Coverage 68.73% 68.06% -0.68%
==========================================
Files 165 166 +1
Lines 14001 14161 +160
==========================================
+ Hits 9624 9639 +15
- Misses 4377 4522 +145 ☔ View full report in Codecov by Sentry. |
monkeysession.setattr( | ||
"tardis.visualization.widgets.custom_abundance.is_notebook", | ||
lambda: True, | ||
) |
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.
Was the issue for Convergence plots or for the custom abundance widget?
Also, I think we should raise an error but I also would like to hear what others say about this. If we do so, maybe then we would want a test that mimics the command line and see if it raises an error?
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.
Was the issue for Convergence plots or for the custom abundance widget?
You're right. I just noticed that the issue was created for the convergence plots. But the code was also visible for the three Widgets, so I made the changes.
I will add the checks for command-line in the Convergence plots.
Also, I think we should raise an error but I also would like to hear what others say about this. If we do so, maybe then we would want a test that mimics the command line and see if it raises an error?
Raising an error would definitely be a more prudent way to handle it.
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.
@atharva-2001 I have added a check where RuntimeError is raised if run_tardis
is called from the command line and if show_convergence_plots
is set to True (which was the only case where the code was being printed).
The default value of show_convergence_plots
was set to True in simulation/base.py
but False in tardis/base.py
. I changed the former to False as well, otherwise the tests which simply call Simulation.from_config
using the default as True would've failed.
c6e673a
to
543e3e0
Compare
@atharva-2001 Can you take a look at this please and let us know what you think? |
1 similar comment
@atharva-2001 Can you take a look at this please and let us know what you think? |
4eadfb9
to
ed82371
Compare
📝 Description
Type: 🪲
bugfix
resolves
#1976The widgets displayed code when called from the command-line as shown in the issue above. This PR adds a check so that the
display()
methods in the widgets run only for notebooks. When called from command-line, a message "Please use a notebook to display the widget" is printed.For the tests, I have used a pytest.Monkeypatch context manager to use a session-scoped fixture and mock the
is_notebook()
response.📌 Resources
Monkeypatch.context
🚦 Testing
How did you test these changes?
☑️ Checklist
build_docs
label@wkerzendorf @atharva-2001