CroatianMeteorNetwork / RMS

RPi Meteor Station
https://globalmeteornetwork.org/
GNU General Public License v3.0
169 stars 47 forks source link

Invalid ECSVs files from SkyFit2 #225

Open hdevillepoix opened 8 months ago

hdevillepoix commented 8 months ago

SkyFit2's saveECSV() function does not save valid ECSV.

Problems comes from hard-coded columns declaration, not matching the CSV header line:

# datatype:
# - {name: datetime, datatype: string}
# - {name: ra, unit: deg, datatype: float64}
# - {name: dec, unit: deg, datatype: float64}
# - {name: azimuth, datatype: float64}
# - {name: altitude, datatype: float64}
# - {name: mag_data, datatype: float64}
# - {name: x_image, unit: pix, datatype: float64}
# - {name: y_image, unit: pix, datatype: float64}

VS

datetime,ra,dec,azimuth,altitude,x_image,y_image,integrated_pixel_value,mag_data

https://github.com/CroatianMeteorNetwork/RMS/blob/c42ec82ece5b6d4a8bdbe4e618998b76358fcea2/Utils/SkyFit2.py#L5318

Easy permanent fix would be to use astropy's one-liner Table write function: .write(ecsv_file_path, format='ascii.ecsv', delimiter=',', overwrite=True) Adds a dependency, but passes on the burden to Astropy to write standard ECSV. More than happy to issue a PR if the astropy dependency is acceptable.

May also build a case for having a GFE linter in https://github.com/UKFAll/standard ?

dvida commented 8 months ago

Hi Hadrien, Thank you for bringing this to my attention. I will try to fix it ASAP. As far as I understand, I only have to modify the header so that column names are in the correct order and that they contain all column declarations? I would love to add astropy, but there are some old camera stations running Raspberry Pi 3s and Python 2 on which it's not possible to install astropy. Because of this I have to be very careful about introducing new dependencies. There is also an issue remotely installing new dependencies on 1000+ cameras across the world. It is automated, but if something goes wrong we have a big problem on our hands.

hdevillepoix commented 8 months ago

Hey Denis,

Yeah as far as I can tell, for reading the file with Astropy's implementation of ECSV, it's just the header block that's the issue.

The STIL/Topcat ECSV implementation has got an additional issue with spaces in the numerical columns. The RA column for example: it knows that it should be a double float from the header declaration. But fails to parse the rows that have leading space(s) for that column (tested on STIL Version 4.1-4).

Interestingly, the same is not true when you remove the ECSV specific # header lines, and tell STIL/Topcat to use the CSV reader. The spaces are not an issue then.

For completeness: Pandas doesn't support ECSV, but doesn't care and will happily read it like so:

pandas.read_csv('SkyFit2_generated.ecsv', comment='#')

100% hear you on software updates on remote old hardware, I know all too well how much of a pain that is :P