search

caracal-pipeline / caracal

Containerized Automated Radio Astronomy Calibration (CARACal) pipeline
GNU General Public License v2.0
28 stars 6 forks source link

Update region module for ddcal #1439

Closed Athanaseus closed 1 year ago

Athanaseus commented 2 years ago

resolves #1438 resolves #1429

paoloserra commented 1 year ago

retest this please

bennahugo commented 1 year ago

Just a note there is definitely breakage in newer Regions parsing. Regions files that read fine with DS9 fail to open due to some coordinate system specification issues in the comment lines. I tried this fix for cubical itself, but I've decided to downgrade and revert to previous versions, see https://github.com/ratt-ru/CubiCal/pull/465

KshitijT commented 1 year ago

Just a note there is definitely breakage in newer Regions parsing. Regions files that read fine with DS9 fail to open due to some coordinate system specification issues in the comment lines. I tried this fix for cubical itself, but I've decided to downgrade and revert to previous versions, see ratt-ru/CubiCal#465

Bleh, maybe it's time to redesign this worker from scratch..

bennahugo commented 1 year ago

I've just spotted there is an issue in crystalball as well due to this. Will submit a downgrade PR now

bennahugo commented 1 year ago

Just a note there is definitely breakage in newer Regions parsing. Regions files that read fine with DS9 fail to open due to some coordinate system specification issues in the comment lines. I tried this fix for cubical itself, but I've decided to downgrade and revert to previous versions, see ratt-ru/CubiCal#465

Bleh, maybe it's time to redesign this worker from scratch..

I still think regions files are the way to go -- they are certainly much easier to work with than casa latices. We just need to be careful with the regions parsers we use

Athanaseus commented 1 year ago

Thanks @bennahugo, then it makes sense to roll this back and just pin it to the previous version of regions.

Also, I notice that the latest regions require python>=3.8. .ie. users running lower versions of python will get previous versions of regions.

KshitijT commented 1 year ago

Do not merge (see comments by @bennahugo and @Athanaseus above).

KshitijT commented 1 year ago

@Athanaseus , please update the branch and then it should be good to go.

KshitijT commented 1 year ago

Athanaseus dismissed KshitijT’s stale review via b150526 1 hour ago.

😢

KshitijT commented 1 year ago

@Athanaseus please merge when ready.