search

astropy / regions

Astropy package for region handling
https://astropy-regions.readthedocs.io
BSD 3-Clause "New" or "Revised" License
61 stars 54 forks source link

Region serialization formats: STC-S #21

Open keflavich opened 8 years ago

keflavich commented 8 years ago

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 commented 8 years ago

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.

timj commented 8 years ago

@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

astrofrog commented 8 years ago

@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)

dsberry commented 8 years ago

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

timj commented 8 years ago

@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).

cdeil commented 8 years ago

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

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?

at88mph commented 6 years ago

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.

cdeil commented 6 years ago

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.

at88mph commented 6 years ago

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.

cdeil commented 6 years ago

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.

at88mph commented 6 years ago

Understood. thanks @cdeil .

at88mph commented 6 years ago

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.

timj commented 6 years ago

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.

keflavich commented 1 year ago

@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.

at88mph commented 1 year ago

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.

keflavich commented 1 year ago

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!

fxpineau commented 9 months ago

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)