Fixed some CRTF write functionality

[JonahDW]

Changed writing to CRTF files such that CASA no longer throws an error because of a trailing comma in each line. Defining global coordinate system is now functional, and utilizes the mapping as defined in the script, along with some other changes.

Also commented out a piece of code that cut the sizes of ellipses in half, which resulted in ellipses being half the size one would expect them to be in the written file.

[JonahDW]

The reason ellipse sizes were cut in half was that the sizes were doubled when reading in (Though I'm not sure which one came first and why...). This has caused great suffering testing the code. All these lines are commented out now.

[keflavich]

Unfortunately most of the changes disagree with the specification document - we need explicit reasoning for any code that does not match that document. There can be reasons, since CASA doesn't actually implement its own spec perfectly, but most of these changes are not warranted.

[JonahDW]

The specification document you link has last been updated in 2016, I think the following link:
https://casa.nrao.edu/casadocs/casa-5.4.1/image-analysis/region-file-format
Is more recent, and this is what I follow. Following the specifications on this page motivated some of the changes, and are what works in recent CASA versions.

[keflavich]

This one is still more recent:
https://casa.nrao.edu/casadocs/casa-5.6.0/imaging/image-analysis/region-file-format
unfortunately, they don't have a changelog.

That said, I'm not sure there are any. The quote about ellipse bmaj/bmin being half-width/half-height remains the same in the latest version(s)

[keflavich]

See #314 for a simple and immediate fix to the problem you highlighted, but this PR is still valid as it does much more - we just need a little additional work to support both global and non-global frames.

[keflavich]

@JonahDW how are you testing whether the region works in CASA? are you just passing it to tclean, or have you found a lighter task that can still report the region's validity?

[JonahDW]

@JonahDW how are you testing whether the region works in CASA? are you just passing it to tclean, or have you found a lighter task that can still report the region's validity?

For the moment I'm just passing it to tclean with niter=1 and just running it as light as possible. I haven't found a quicker way to do it at the moment.

[keflavich]

Sadly, I think that's the best way to do it.

Could you verify that #314 solves your original issue, then rebase this PR against it? I added a more specific test for your issue there.

[JonahDW]

Sadly, I think that's the best way to do it.

Could you verify that #314 solves your original issue, then rebase this PR against it? I added a more specific test for your issue there.

Sorry, I'm hitting the limits of my git knowledge here. What would be the best way of getting your PR, checking if it works, and then rebasing it with my PR?

[keflavich]

ahh, no worries. This pull request is from your master branch. I'm not sure this is the most straightforward, but this is what I would do. In your regions directory:

git remote add keflavich git@github.com:keflavich/regions.git
git fetch keflavich
git checkout keflavich/issue312
pip install -e .

then, test that the region file works.

If it does, ping me on #314 and we can (maybe) merge it.

Then, locally:

git checkout master
git rebase -i keflavich/issue312
# do the interactive rebasing stuff here
git push -f origin master

I would suggest joining the astropy slack and asking for tips in the general channel if any of this doesn't make sense... I'm not sure my practice matches best practice here.

Note that if we merged #314 first, you would instead git rebase -i upstream/master, where upstream would be git remote add upstream git@github.com:astropy/regions.git.

Anyway, if the rebasing causes you issues, definitely ask for help. I hope checking out my branch should be more straightforward. You can also use the "view commandline instructions" approach

that github offers.

[JonahDW]

While testing, I'm finding that while CASA expects an ellipse shape to be:
ellipse[[x, y], [bmaj, bmin], pa]
The output is written like
ellipse[[x, y], [bmin, bmaj], pa]
It seems CASA interprets these very strictly and doesn't care which axis is actually the largest. What's even more puzzling is that in their example they seem to go against their own syntax and write
-ellipse[[17:51:03.2, -45.17.50], [0.25deg, 1.34deg], 45rad], range=[1.420GHz, 1.421GHz], corr=[Q], color=green, label=’Removed this’

This results in having to pass the major axis to width and minor axis to height in order for the mask to be correct in the imaging, which is rather confusing. It's not quite clear to me how to best fix this, I suppose the easiest way to do it would be to change the crtf string the to_crtf function:
'ellipse': '{0}ellipse[[{1:FMT}deg, {2:FMT}deg], [{3:FMT}RAD, {4:FMT}RAD], {5:FMT}deg]',
Where swapping 3 and 4 would do the trick. Then we just have to make sure this is consistent with reading in as well.

[keflavich]

Yeah, I think swapping in the CRTF writing makes sense. Definitely note this in a comment!

It would be really, really helpful if we could get some legit tests up and running. I know your data are proprietary, but do you think we can test this with one of the casaguides datasets? If we can, we could set up an extra set of tests using casa-6 (pip installable version of casatools & casatasks) to verify that what we're doing works as anticipated. Basically, we'd run tclean with niter=1 and verify that the right pixels are added to the mask.

It would also be worth raising an issue (a helpdesk ticket) with the CASA helpdesk, since they really should obey their own specification...

[JonahDW]

Of course, testing in CASA would be the best way to ensure everything is working correctly, however the datasets in the casaguides are still fairly large (~GB size). Would performing such a test not require a download of such a dataset? That would make matters a tad more complicated I reckon.

[JonahDW]

Another option is to use some of the tools described at the bottom on the following page
https://casaguides.nrao.edu/index.php/CASA_Region_Format_Examples

Which only requires a CASA image to work. This would provide an easy way to check if the file is valid, though it's harder to check if the correct pixels are masked this way.

[keflavich]

For CASA testing, we could set up a small artificial MS on the fly to avoid downloading data. The image approach is a good one, but as you said, it's limited.

[JonahDW]

For CASA testing, we could set up a small artificial MS on the fly to avoid downloading data. The image approach is a good one, but as you said, it's limited.

Okay, I believe I have found a quick and easy way to do this now. Using the simulation tool in CASA a small measurement set (less than an MB) can be created. This should make it easy to test the masking with tclean. I'll see if this can be easily worked out into a test, one would have to have some way to interpret if CASA exits on an error as well of course.

[keflavich]

That's great!

We'll need to add some stuff to the .travis config to install the casa-6 tasks, but we've done that before in spectral-cube so it shouldn't be too hard.

[JonahDW]

I've added a file containing a start on testing the functionality of the masks with CASA. I took some inspiration from the spectral-cube docs, however due to package buggery installing casatools/tasks has been a challenge, so I can't entirely be sure that it all functions. Would very much appreciate feedback on what is done so far.

[keflavich]

Just a note for us as we build this - importing casatasks will import matplolib and try to start a background, so we need to enable a headless X-server on travis.

[keflavich]

this should probably be assert all(mask_array == ref_mask), right? We're checking that all booleans are the same value?

[JonahDW]

Should I make some changes based on the coverage checks? I'm not entirely sure what it is that it's trying to tell me..

[astrofrog]

@JonahDW - no don't worry about the coverage checks!