regions icon indicating copy to clipboard operation
regions copied to clipboard

Region serialization formats: STC-S

Open keflavich opened this issue 9 years ago • 17 comments

A region format that exists and that we could implement is the IVOA (I think?) supported STC-S, as described here: http://adsabs.harvard.edu/abs/2010ASPC..434..213B

There was some discussion about putting the STC format into yaml files, which might make the i/o very nice & easy.

keflavich avatar Mar 27 '16 21:03 keflavich

cc @timj do you have a (python) parser/writer for this available? Also, we'd like example files to test on if you have them.

keflavich avatar Mar 27 '16 21:03 keflavich

@keflavich Sorry. It's all C code inside pyast. @dsberry do you have test STC-S region examples other than the one in that paper? I see a couple in teststcschan.f inside AST but they aren't easy to extract from there.

Regarding STC-S: http://www.ivoa.net/documents/Notes/STC-S/20091030/NOTE-STC-S-1.33-20091030.html

timj avatar Mar 27 '16 23:03 timj

@timj - does pyast expose the readers from a Python API though? We could just add a dependency on pyast for now for this kind of file format? (no point in reinventing the wheel)

astrofrog avatar Mar 28 '16 01:03 astrofrog

pyast includes a wrapper for the StcsChan class - see http://www.starlink.ac.uk/docs/sun211.htx/sun211ss487.html. For an example of its usage, see https://gist.github.com/dsberry/9a0dc13c86e8db9f4df1

You may also be interested in an on-line demo of the StcsChan class (recently resurrected) : http://starlink.eao.hawaii.edu/cgi-bin/ast/stcs-demo

dsberry avatar Mar 28 '16 08:03 dsberry

@dsberry thanks for the demo. I imagine that would be useful.

@astrofrog regarding using AST to parse the regions. I'm not sure. You could in theory get the compound region from AST and split it into constituent regions and then use GetRegionPoints to work out what each individual region is. Not sure how feasible that is. In some sense pyast already does a lot of what is needed for region handling already (it looks like GetRegionMesh would be a great way to get the points required for a FootPrint).

timj avatar Mar 29 '16 17:03 timj

It looks like a lot of thought has gone into STC-S and having this in astropy.regions would be a big win-win:

  • More widespread adoption for STC-S.
  • A well thought-out data model and serialisation format for astropy.region.

Note: everything is very preliminary in astropy.regions, the data model and API is up for debate. IMO we should try to get this ready in the next few months and propose inclusion in the Astropy core before the Astropy 1.3 release, or if we don't manage that, then the one after that.

@timj @dsberry - Do you or someone else have time to contribute this to astropy.regions? Or alternatively, provide a detailed instructions how it should be implemented (ideally in manageable steps that are each a few days or week at most of work) that someone else could possibly pick up and then we ask for help on astropy-dev on this?

cdeil avatar Jul 29 '16 08:07 cdeil

I noticed this issue is still open, so I hope it's OK to append to it. We have a need for an STC-S parser in Python to support some cutout functionality. Did an implementation of sorts ever get worked out? I have started one on my GitHub, but it currently on parses Circles and Polygons.

Can I contribute to the regions library?

Cheers.

at88mph avatar Sep 20 '18 21:09 at88mph

I think we have nothing for STC-S and it's clearly in scope. Of course, depending on what changes it implies for the package, we have to talk, but basically: it would be very welcome.

Do you already know if it will be just another serialisation format like how we already have e.g. DS9? https://astropy-regions.readthedocs.io/en/latest/#user-documentation Or will it require changes to the regions data model and Python classes?

Another concern is license. Astropy and regions is BSD-3 and here I see GPL: https://github.com/at88mph/opencadc_stc/blob/master/LICENSE We will not take GPL. So if you wrote that and can re-license, or have the right to contribute under our license, great! If that originates from some GPL codebase that others were involved in, we might not be able to take it.

cdeil avatar Sep 20 '18 21:09 cdeil

I wrote it and it can definitely be re-licensed. It only depends on astropy.

I would see it as a format akin to the CRTF and DS9 ones. It will be an addition and no core changes should be required, unless some refactoring regarding the data model (as mentioned above) should happen first.

Is there a code convention contribution guide?

Thanks.

at88mph avatar Sep 20 '18 21:09 at88mph

Great!

We didn't write any contribute docs, but pretty much everything is the same as for Astropy, and they have super extensive docs: https://astropy.readthedocs.io/en/stable/#developer-documentation Feel free to ask any time on Github though if something isn't clear.

Do you have an estimate how much code / work this addition is?

I think it's significant and we should try to make sure the addition happens efficiently with your and our time. Personally I don't know about STC-S and have no time to do PR review. So if someone here could do that or at least give some feedback on PRs (@keflavich @timj @dsberry ?), please leave a comment. Also: @at88mph - please have a look at our regions classes, and think about whether it's possible / a good idea to make the addition as a series of smallish PRs, or if it should or has to be one large PR.

cdeil avatar Sep 20 '18 21:09 cdeil

Understood. thanks @cdeil .

at88mph avatar Sep 20 '18 21:09 at88mph

Sorry. I've been told that STC-S never made it to a standard and was rejected. We will no longer be pursuing this. My apologies.

at88mph avatar Oct 01 '18 16:10 at88mph

The note is here http://www.ivoa.net/documents/Notes/STC-S/

Obviously many people have implemented it and I'm pretty sure JAC is sending STC-S region strings to CADC. I wonder why it never got formalized. It was useful.

timj avatar Oct 01 '18 16:10 timj

@at88mph Any chance you would be willing to port your implementation over here now? (following up on this conversation on astroquery)

Given that STC-S implementations exist in the wild, it is a de facto standard, so I'd like regions to include an implementation of at least a reader and ideally a serializer too.

keflavich avatar Mar 20 '23 13:03 keflavich

I'm sorry @keflavich , I just saw your last note. Is this issue still relevant? I'm not sure if there are other implementations more recent or not.

at88mph avatar Sep 13 '23 18:09 at88mph

Yes, this issue is still relevant - we still don't, afaik, have a mechanism to read or write STC-S regions in this module, which is a feature we'd like!

keflavich avatar Sep 13 '23 18:09 keflavich

I know that so far astropy has been reluctant to integrate Rust code. But, FWIW, I made a STC-S parser in Rust that could be used to transform a STC-String into a Python dictionary (from the JSON output of the parser), ensuring a part of the work toward a STC-S to Region code. The license is MIT, the code available on both github and crates.io. Let me know whether you may be interested or not.

(This parser is used in the new -- still in test -- STC-S to MOC feature in MOCPy, which is an astropy affiliated package)

fxpineau avatar Dec 14 '23 16:12 fxpineau