Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pyregion... but not really #163

Closed
jhunkeler opened this issue Jun 26, 2018 · 6 comments
Closed

Pyregion... but not really #163

jhunkeler opened this issue Jun 26, 2018 · 6 comments
Labels
code cleanup Code refactoring, cleanup of temporary code question

Comments

@jhunkeler
Copy link
Contributor

@mcara @stsci-hack This TODO is four years old give or take. If the fixes mentioned were not implemented upstream for whatever reason can't we put in a PR on astropy/pyregion.git to get this long standing issue resolved?

#TODO: we replaced pyregion.write with our implementation
# of the write (code from _regwrite) in the locally maintained
# pyregion until these changes get to be implemented in the
# publicly available release of pyregion.

Note: spacetelescope/pyregion.git is marked as an archived repo. As far as anyone knew our special version wasn't used by anything (cc @SaOgaz)

@jhunkeler jhunkeler added question code cleanup Code refactoring, cleanup of temporary code labels Jun 26, 2018
@mcara
Copy link
Member

mcara commented Jun 26, 2018

@jhunkeler wrote:

Note: spacetelescope/pyregion.git is marked as an archived repo. As far as anyone knew our special version wasn't used by anything.

My understanding was that even though it is archived, it will still be used by drizzlepac for the time being. The reason for this was given to @SaOgaz in an e-mail sent on 05/18/2018:

I am not sure what this “archival” means. For now we do not need to make any changes/fixed to our copy of pyregion so you can continue keeping pyregion archived if you prefer so. We can un-archive it later if need be. Fundamentally astropy/pyregion removed support not only for tabular WCS corrections (by removing WCS support) but also they removed support for SIP distortions - and this is too much [too large of an error]!

So, the link that you provide to the comment in the drizzlepac is not the only issue with the newest incarnation. Fundamentally, astropy/pyregion is a stripped-down version of the older package. I am not even sure it does anything better than the old one (except for not relying on kapteyn and using astropy instead - meh, IMO). Even worse, it looks like astropy intends to deprecate pyregion anyway (and that is why, probably, there isn't much activity there): astropy/pyregion#66 (comment).

I can write my own function for writing regions re right way, but removed support for caller providing their own WCS - I can't fix that.

I also do not understand all this rush with archiving repositories anyway. What is the benefit of having spacetelescope/pyregion archived? I was hoping that it will not be harmful, but it looks like it was.

@jhunkeler
Copy link
Contributor Author

jhunkeler commented Jun 26, 2018

On Slack @mcara suggested creating a stregion repo and changing the dependencies of drizzlepac to use it instead.

From my seat this is the only way to circumvent the current problem. If a user installs drizzlepac via pip they're going to end up with the Astropy release of pyregion, not ours. If we change the requirement to be stregion and throw it up on pypi, the user will always receive our version of the code (and the package will not be ambiguously named).

@mcara
Copy link
Member

mcara commented Jun 26, 2018

@jhunkeler Let see if it would be possible to fast-track astropy/pyregion#131 Maybe then we can start using astropy/pyregion though I am fearful of future changes to the package and API. If it is not stable enough, it will be more of a headache. So, maybe it still would make sense to switch to our own code. Also, I can't be sure that astropy/pyregion has kept all the capabilities of the older package and that it will be easy enough to transition to astropy/pyregion (i.e., I will need to test that everything works as it should, and this is time consuming). What do you think @jhunkeler ?

@jhunkeler
Copy link
Contributor Author

jhunkeler commented Jun 26, 2018

Regarding what was discussed in astropy/pyregion#131 ... do you think https://github.com/astropy/regions is a viable alternative?

Side-note: All of these different XYZregion packages are going to confuse me at some point.

@mcara
Copy link
Member

mcara commented Jun 26, 2018

@jhunkeler I did not spend much time - just a quick look ... and it seems that it might be. However, it is not a drop-in replacement and it will definitely take more time to incorporate.

@mcara
Copy link
Member

mcara commented Jun 27, 2018

@jhunkeler Thank you very much for stregion and for really really great README.md!!! 🥇

mcara added a commit to mcara/astroconda-contrib that referenced this issue Jun 28, 2018
jhunkeler pushed a commit to astroconda/astroconda-contrib that referenced this issue Jun 29, 2018
* Switch drizzlepac to using stregion

See spacetelescope/drizzlepac#163

* Change pyregion to stregion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Code refactoring, cleanup of temporary code question
Projects
None yet
Development

No branches or pull requests

2 participants