Add position-velocity-diagram extraction & plotting notebook

[keflavich]

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]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[adrn]

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

[keflavich]

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

[keflavich]

@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]

@keflavich --Front material looks good!

[jonathansick]

@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]

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

[eblur]

"We can also build extraction regions by supplying coordinates defined in an astropy.coordinates.SkyCoord to pvextractor.Path "

Reply via ReviewNB

[eblur]

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]

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:

[eblur]

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]

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

[jonathansick]

Suggested change

	<<<<<<< HEAD
	pyvo==1.2.0
	=======
	pvextractor
	>>>>>>> a911f0e... add pvextractor to reqs
	pyvo==1.2.0
	pvextractor

@keflavich Could you resolve this merge conflict here in requirements.txt? I think that should do it.

[jonathansick]

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]

Suggested change

	"name": "python39"
	"name": "python3"

Would you mind making this single change, directly on the JSON (without saving it against via the Jupyter app?)

[jonathansick]

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]

That's a pvextractor version issue

[jonathansick]

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]

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).