astropy / astropy-tutorials

Tutorials for the Astropy Project
BSD 3-Clause "New" or "Revised" License
291 stars 176 forks source link

Add position-velocity-diagram extraction & plotting notebook #503

Closed keflavich closed 2 years ago

keflavich commented 2 years ago

This is a new tutorial on extracting & plotting position-velocity diagrams.

It could use review from a non-specialist.

This is also submitted to radio-astro-tools/tutorials, which we're kind of treating as a sandbox for learn.astropy.

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

adrn commented 2 years ago

Sorry for the build failures - I just fixed those in #505 - do you mind rebasing on main?

keflavich commented 2 years ago

I punted on the optional activity; I'd love to add it, but it's hard to find an appropriate data set

keflavich commented 2 years ago

@kelle this is ready for final review

@e-koch your review on the changes to the front material would be helpful (I added some use case text)

@jonathansick your review on the technical aspects - should we squash and merge, for example? - would be helpful

e-koch commented 2 years ago

@keflavich --Front material looks good!

jonathansick commented 2 years ago

@keflavich I'm seeing a tutorial build error in GitHub Actions. Here's an extract of the logs:

------------------
16
ax = pl.subplot(111, projection=cube.wcs.celestial)
17
ax.imshow(cube[25].value)
18
{'execute_kwargs': {'kernel_name': '', 'timeout': 600, 'allow_errors': False}, 'convert_kwargs': {}, 'notebooks': ['tutorials/position-velocity-diagrams/PVDiagramPlotting.ipynb'], 'build_path': '.', 'flatten': True, 'overwrite': False, 'exclude_pattern': None, 'include_pattern': None, 'verbosity': 1, 'quietness': 0}
19
path.show_on_axis(ax, spacing=1, color='r')
20
ax.set_xlabel(f"Right Ascension [{cube.wcs.wcs.radesys}]")
21
ax.set_ylabel(f"Declination [{cube.wcs.wcs.radesys}]")
22
------------------
23

24
---------------------------------------------------------------------------
25
AttributeError                            Traceback (most recent call last)
26
Input In [5], in <module>
27
      1 ax = pl.subplot(111, projection=cube.wcs.celestial)
28
      2 ax.imshow(cube[25].value)
29
----> 3 path.show_on_axis(ax, spacing=1, color='r')
30
      4 ax.set_xlabel(f"Right Ascension [{cube.wcs.wcs.radesys}]")
31
      5 ax.set_ylabel(f"Declination [{cube.wcs.wcs.radesys}]")
32

33
AttributeError: 'Path' object has no attribute 'show_on_axis'
34
AttributeError: 'Path' object has no attribute 'show_on_axis'
35

36
Traceback (most recent call last):
37
  File "/opt/hostedtoolcache/Python/3.9.9/x64/bin/nbcollection", line 33, in <module>
38
    sys.exit(load_entry_point('nbcollection==0.3.dev8+g516c172', 'console_scripts', 'nbcollection')())
39
  File "/opt/hostedtoolcache/Python/3.9.9/x64/lib/python3.9/site-packages/nbcollection/__main__.py", line 33, in main
40
    commands[parsed.command](args)
41
  File "/opt/hostedtoolcache/Python/3.9.9/x64/lib/python3.9/site-packages/nbcollection/commands/execute.py", line 24, in execute
42
    nbcollection.execute()
43
  File "/opt/hostedtoolcache/Python/3.9.9/x64/lib/python3.9/site-packages/nbcollection/converter.py", line 156, in execute
44
    raise RuntimeError(f"{len(exceptions)} notebooks raised unexpected "
45
RuntimeError: 1 notebooks raised unexpected errors while executing cells: ['PVDiagramPlotting.ipynb'] — see above for more details about the failing cells. If any of these are expected errors, add a Jupyter cell tag 'raises-exception' to the failing cells.

Is path not the object we're expecting?

keflavich commented 2 years ago

Huh, I think this could be a version issue! I'll check

eblur commented 2 years ago

Overall, this looks good to me! I was confused by the use of the term "path" and asked for clarification around that. However, if "path" is the standard terminology used in radio astronomy to describe extraction regions (if I understand it correctly), then that is okay.

keflavich commented 2 years ago

path is a technical term introduced in this document. I've added a little clarifying text, but there's not much more to be said in words. It is not a "region" in the sense I use that term (like a ds9 sky region), it is a drawn line in 2 dimensions. But the point is to show what it is.

I welcome additional suggestions on how to present this.

Here's the figure showing the extraction path drawn on the figure: image

eblur commented 2 years ago

Thanks @keflavich , the image does help. I think it's good enough.

Regarding the build issues, it looks like you need to rebase this branch like you did for #504 . If all is well after that, we'll merge.

jonathansick commented 2 years ago

@keflavich Can you try rebasing this now? We've pinned pyvo so that should so the build issue.

jonathansick commented 2 years ago

The requirements.txt is good now. The old issue with the python39 kernel name is back:

[nbcollection (DEBUG)]: Executing notebook 'PVDiagramPlotting.ipynb' ⏳
[nbcollection (ERROR)]: Notebook 'PVDiagramPlotting.ipynb' errored: No such kernel named python39

I thought this was supposed to be resolved by the nbstripout hook in pre-commit? Apparently not?

jonathansick commented 2 years ago

Thanks for the switch to python3 kernel; that's got us going further. Now the issue is

[nbcollection (DEBUG)]: Executing notebook 'PVDiagramPlotting.ipynb' ⏳
[nbcollection (ERROR)]: Notebook 'PVDiagramPlotting.ipynb' errored ❌
[nbcollection (ERROR)]: Notebook 'PVDiagramPlotting.ipynb' errored: An error occurred while executing the following cell:
------------------
ax = pl.subplot(111, projection=cube.wcs.celestial)
ax.imshow(cube[25].value)
path.show_on_axis(ax, spacing=1, color='r')
ax.set_xlabel(f"Right Ascension [{cube.wcs.wcs.radesys}]")
ax.set_ylabel(f"Declination [{cube.wcs.wcs.radesys}]")
------------------

---------------------------------------------------------------------------
[24](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:24)
AttributeError                            Traceback (most recent call last)
[25](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:25)
Input In [5], in <module>
[26](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:26)
      1 ax = pl.subplot(111, projection=cube.wcs.celestial)
[27](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:27)
      2 ax.imshow(cube[25].value)
[28](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:28)
----> 3 path.show_on_axis(ax, spacing=1, color='r')
[29](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:29)
      4 ax.set_xlabel(f"Right Ascension [{cube.wcs.wcs.radesys}]")
[30](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:30)
      5 ax.set_ylabel(f"Declination [{cube.wcs.wcs.radesys}]")
[31](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:31)

[32](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:32)
AttributeError: 'Path' object has no attribute 'show_on_axis'
[33](https://github.com/astropy/astropy-tutorials/runs/5306271287?check_suite_focus=true#step:6:33)
AttributeError: 'Path' object has no attribute 'show_on_axis'

This is familiar; wasn't it already fixed?

keflavich commented 2 years ago

That's a pvextractor version issue

jonathansick commented 2 years ago

We're installing pvextractor 0.2, which is the latest version on PyPI. Do we need to be installing pvextractor from GitHub to get the right Path API?

i.e. change pyextractor in requirements.txt to something like:

git+https://github.com/radio-astro-tools/pvextractor.git#egg=pvextractor

(that tracks the master branch)

Or could you do a new release to PyPI?

adrn commented 2 years ago

To close the loop: @keflavich did a new release of pvextractor, so the change @jonathansick asked for about doesn't need to be added (and CI is now passing).