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

Fix astroquery.utils.system_tools.in_ipynb() #2289

Merged
merged 1 commit into from
Feb 23, 2022

Conversation

eerovaher
Copy link
Member

The purpose of astroquery.utils.system_tools.in_ipynb() is to recognize if it is being called from a .ipynb notebook or not, but the current function seems to always return False. The function in this pull request returns False if it is called from ipython, but True if called from jupyter notebook or jupyter lab, at least for the versions I'm using.

ipython --version
7.19.0
jupyter-notebook --version
6.1.5
jupyter-lab --version
3.0.16

@keflavich
Copy link
Contributor

I don't know if it's safe to rely on an environmental variable for this sort of check... but clearly the "robust-looking" approach we took wasn't. I'm OK with trying this.

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #2289 (8cec4e5) into main (08e57d7) will increase coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2289      +/-   ##
==========================================
+ Coverage   62.75%   62.77%   +0.02%     
==========================================
  Files         130      130              
  Lines       16835    16828       -7     
==========================================
  Hits        10564    10564              
+ Misses       6271     6264       -7     
Impacted Files Coverage Δ
astroquery/utils/system_tools.py 75.00% <0.00%> (+22.82%) ⬆️

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 08e57d7...8cec4e5. Read the comment docs.

@bsipocz
Copy link
Member

bsipocz commented Feb 8, 2022

I wonder if we need this as a separate function or could just use the suggested workaround directly in query.py?

@eerovaher
Copy link
Member Author

I don't know if it's safe to rely on an environmental variable for this sort of check...

An alternative might be

try:
    return 'connection_file' in get_ipython().config['IPKernelApp']
except NameError:
    return False

But I don't know if there's actually any difference in practice.

@bsipocz bsipocz added the utils label Feb 23, 2022
@bsipocz bsipocz added this to the v0.4.6 milestone Feb 23, 2022
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

I still feel this may be now an overshoot for a separate function, but in either case the fix is working and an improvement itself.

@bsipocz
Copy link
Member

bsipocz commented Feb 23, 2022

Thanks @eerovaher!

@bsipocz bsipocz merged commit 441ee5f into astropy:main Feb 23, 2022
@eerovaher eerovaher deleted the fix-in-ipynb branch March 8, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants