Skip to content

Fixes and enhancements to ShapeList.write() - #33

Merged
leejjoon merged 1 commit into
astropy:masterfrom
mcara:write-fixes-enhancements
Aug 21, 2014
Merged

Fixes and enhancements to ShapeList.write()#33
leejjoon merged 1 commit into
astropy:masterfrom
mcara:write-fixes-enhancements

Conversation

@mcara

@mcara mcara commented May 20, 2014

Copy link
Copy Markdown
Contributor

The following issues are being addressed in the proposed changes:

  1. pyregion.ShapeList.write crashes when saving regions without comments [bug fix];
  2. pyregion.ShapeList.write converts exclude ("-") DS9 regions to regular ("+") region when saving region files [bug fix];
  3. pyregion.ShapeList.write cannot save region files that contain regions with mixed Coordinate Systems (e.g., image, sky, etc.) Proposed here changes will allow saving region lists with mixed coordinate systems. [enhancement]

This replaces a part of the PR #29.

@mcara
Fixes and enhancements to ShapeList.write()
87e78c5 
The following issues are being addressed in the proposed changes:

1. `pyregion.ShapeList.write` crashes when saving regions without
comments [bug fix];

2.  `pyregion.ShapeList.write` converts exclude ("-") DS9 regions to
regular ("+") region when saving region files [bug fix];

3.  `pyregion.ShapeList.write` cannot save region files that contain
regions with mixed Coordinate Systems (e.g., image, sky, etc.) Proposed
here changes will allow saving region lists with mixed coordinate
systems. [enhancement]
@mcara mcara mentioned this pull request May 20, 2014
Closed
@leejjoon

leejjoon commented May 28, 2014

Copy link
Copy Markdown
Contributor

Can you check if this PR conflicts with #30?
If it does, could you work with @keflavich to come up with an unified PR.

@mcara

mcara commented Jul 6, 2014

Copy link
Copy Markdown
Contributor Author

@leejjoon @keflavich
I want to apologize for the long delay.

I am not sure if I understand correctly the whole set of changes proposed by @keflavich but it seems to me it adds support for new shapes (e.g., projections), can print more attributes (comments), and solves the bug reported in my PR #29.

I am happy to see these additions, however, I think my bug fix can also print the whole comment line (when present). In addition, unlike @keflavich PR, it will not add a comment sign # if no comments are present. Finally, and this is important for me, my PR fixes the issue of pyregion "forgetting" include/exclude attribute of the region (e.g., "-" sign for exclude regions). Finally, my PR allows writing out region files with regions having mixed coordinate systems, e.g., 'image', 'fk5', etc.

I think (please correct me if I am wrong - I am no expert in Github), there are two approaches to dealing with these two PRs (considering that my original PR #29 was the first to fix crashes in the write() function:

  1. You can merge my PR first and then @keflavich can submit another PR modifying the new code to add his functionality;
  2. @keflavich can branch my PR and then I can merge them to my PR and then you can merge my PR (with @keflavich changes) to the master. I must recognize I do not particularly like this second approach mostly because I am not sure how to do this (I have very little experience with Github).

leejjoon added a commit that referenced this pull request Aug 21, 2014
@leejjoon
Merge pull request #33 from mcara/write-fixes-enhancements
2a3a5c4 
Fixes and enhancements to ShapeList.write()
@leejjoon
leejjoon merged commit 2a3a5c4 into astropy:master Aug 21, 2014
@leejjoon

leejjoon commented Aug 21, 2014

Copy link
Copy Markdown
Contributor

I am merging this PR. @keflavich : if this does not solve your problem, please send in a new PR based on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mcara @leejjoon