Skip to content

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

Merged
bsipocz merged 1 commit into
astropy:mainfrom
eerovaher:fix-in-ipynb
Feb 23, 2022
Merged

Fix astroquery.utils.system_tools.in_ipynb()#2289
bsipocz merged 1 commit into
astropy:mainfrom
eerovaher:fix-in-ipynb

Conversation

@eerovaher

Copy link
Copy Markdown
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
Copy Markdown
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

codecov Bot commented Feb 8, 2022

Copy link
Copy Markdown

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

bsipocz commented Feb 8, 2022

Copy link
Copy Markdown
Member

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

@eerovaher

Copy link
Copy Markdown
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

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

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

bsipocz commented Feb 23, 2022

Copy link
Copy Markdown
Member

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.

3 participants