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

How to set observer coordinate in a Map? #4189

Open
dstansby opened this issue May 20, 2020 · 9 comments
Open

How to set observer coordinate in a Map? #4189

dstansby opened this issue May 20, 2020 · 9 comments
Labels
Effort Medium Requires a moderate time investment map Affects the map submodule Package Expert Requires alot of knowledge of the internal structure of SunPy Priority Low Slow action required Refactoring Code changes without affecting API (generally)

Comments

@dstansby
Copy link
Member

I currently have a lot of maps with missing observer coordinate information, and I would like to set this information to the location of the earth at the time the map was observed.

One way of doing this is to manually calculate what the values of the 'hgln_obs', 'hglt_obs', 'dsun_obs' FITS keywords should be, manually add them to the existing header, and create a new map. But I think a user should not have to worry about or know anything about FITS to do this.

Another way of doing this would be giving the observer_coordinate property a setter, and I could then set the observer coordinate to a SkyCoord (and internally sunpy would handle with the FITS stuff). But it has been decided not to support property setters on Map ☹️ (#3911)

So my questions are

  • Should I learn FITS to do this?
  • Should we revise the decision to not have any setters for Map?
  • Is there another way to do this?
@dstansby dstansby added the map Affects the map submodule label May 20, 2020
@ayshih
Copy link
Member

ayshih commented May 20, 2020

One could imagine creating a convenience function or method (e.g., called something like set_map_observer_coordinate()) that does this without it having to be a setter for observer_coordinate.

@ayshih
Copy link
Member

ayshih commented May 20, 2020

Obviously whoever actually writes such a function would need to "learn FITS". =)

@dstansby
Copy link
Member Author

Why a convenience function/method instead of a setter?

@ayshih
Copy link
Member

ayshih commented May 20, 2020

Some reasons to avoid a setter:

  • We may not want users to think of a Map as mutable.
  • We may not want to provide setters for everything, and then it could be confusing to the user which attributes are settable and which ones are not.
  • We may want to return a new Map with the new observer information to make sure that all of the normal machinery is triggered (i.e., to minimize the potential for inconsistencies).

@dstansby
Copy link
Member Author

* We may not want users to think of a Map as mutable.

Why not?

* We may not want to provide setters for everything, and then it could be confusing to the user which attributes are settable and which ones are not.

This seems easily fixed by clear documentation ("This is a list of the properties that can be set").

* We may want to return a new Map with the new observer information to make sure that all of the normal machinery is triggered (i.e., to minimize the potential for inconsistencies).

What's the normal machinery that needs to be triggered? Wouldn't it be possible to trigger it from a setter (which is essentially just a method)?

I don't think there are implementation reasons to choose a setter or a method, but it seems like there are API reasons, the difference being

m.observer_coordinate = my_skycoord

or

new_m = m.set_observer_coord(my_skycoord)

And it sounds like the second method option is the best one, as it preserves the original map.

@Cadair
Copy link
Member

Cadair commented May 21, 2020

There is a lot to unpack here. I am going to focus on the short term things, given the current design we have for map etc. I think the bigger picture issues (setters, mutability etc) can be considered as and when we refactor the metadata handling.

Firstly, I started hacking on addressing #3643 which should be a part of this, although our current design of on-the-fly generated WCSes makes it not really an API solution.

I think having a set_observer_coordinate method might be a good short term solution, however, I do agree that it would be much more Pythonic to use setters. That being said, given the current design of map, that's a can of worms I would rather not open. Also I think exactly the shape of this API depends on the work around #3643 etc.

The main issue I see with implementing something which sets the observer coordinate is maintaining coherence (or at least finding and removing) observer coordinates in different systems in the header. I.e. if you load an AIA map and then set the observer coordinate, ideally all the obs_xxxx keys should be removed and just the HGS ones left or otherwise bad things will happen.

@ayshih
Copy link
Member

ayshih commented May 21, 2020

The main issue I see with implementing something which sets the observer coordinate is maintaining coherence (or at least finding and removing) observer coordinates in different systems in the header. I.e. if you load an AIA map and then set the observer coordinate, ideally all the obs_xxxx keys should be removed and just the HGS ones left or otherwise bad things will happen.

See #4194 for a concept that's been brewing in my brain

@nabobalis nabobalis added Effort Medium Requires a moderate time investment Package Novice Requires little knowledge of the internal structure of SunPy Priority Low Slow action required Refactoring Code changes without affecting API (generally) labels Jul 11, 2020
@dstansby
Copy link
Member Author

I've put code to take a coordinate and return appropriate observer metadata in pfsspy if anyone is interested in adding it to sunpy: https://github.com/dstansby/pfsspy/pull/230/files

@Cadair Cadair added Package Expert Requires alot of knowledge of the internal structure of SunPy and removed Package Novice Requires little knowledge of the internal structure of SunPy labels Sep 23, 2020
@ayshih
Copy link
Member

ayshih commented Sep 24, 2020

#3110 is related, but not a duplicate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort Medium Requires a moderate time investment map Affects the map submodule Package Expert Requires alot of knowledge of the internal structure of SunPy Priority Low Slow action required Refactoring Code changes without affecting API (generally)
Projects
None yet
Development

No branches or pull requests

4 participants