search

askap-vast / vast-pipeline

This repository holds the code of the Radio Transient detection pipeline for the VAST project.
https://vast-survey.org/vast-pipeline/
MIT License
7 stars 3 forks source link

Produce IAU compliant source names #618

Closed marxide closed 2 years ago

marxide commented 2 years ago

Ensures that source names saved in the Source model are IAU compliant by truncating coordinates, instead of rounding. Includes a database migration to update the existing names in the database.

Fixes #617.

marxide commented 2 years ago

@ajstewart I think I've done this right, but if you can think of another coordinate I should add to the unit test please let me know. Maybe I've missed some edge case?

marxide commented 2 years ago
  1. I suspect this code was somewhat inherited from TraP - I wondered if astropy has easy functionality to replace it? Or if there is a reason why we need to write this conversion ourselves I'm forgetting.

I couldn't find one. There's a bit in the coordinates docs that gives an example of converting a coordinate to an IAU formatted string, but it rounds!

  1. I think this will also mean that the parquet sources file will be out of sync for all the runs. I have a feeling the name column is included in that.

I don't see a name column in the sources parquet files.

ajstewart commented 2 years ago
  1. I suspect this code was somewhat inherited from TraP - I wondered if astropy has easy functionality to replace it? Or if there is a reason why we need to write this conversion ourselves I'm forgetting.

I couldn't find one. There's a bit in the coordinates docs that gives an example of converting a coordinate to an IAU formatted string, but it rounds!

Ha, great!

  1. I think this will also mean that the parquet sources file will be out of sync for all the runs. I have a feeling the name column is included in that.

I don't see a name column in the sources parquet files.

Oh yeah, my memory is off. I wonder why we didn't include that in the output? That would be useful I would have thought.

Odd test failures?

marxide commented 2 years ago

Odd test failures?

Aggghhh. I have no idea what's causing these yet. Like last time, they seem unrelated to the PR. Probably another dependency problem? I'll dig deeper later.

ajstewart commented 2 years ago

Aggghhh. I have no idea what's causing these yet. Like last time, they seem unrelated to the PR. Probably another dependency problem? I'll dig deeper later.

Having a quick look, the error comes from handling the fits during the forced extraction. I'm wondering if there was a hiccup with the data download - the wget logs seem to suggest yes. Perhaps try running them again and they might pass.