Integrate Astropy units into the HEK Results table #7059
Conversation
sunpy/net/hek/hek.py
Outdated
|
|
||
| u.add_enabled_aliases({"steradian": u.sr, "arcseconds": u.arcsec, "degrees": u.deg, "sec": u.s}) |
There was a problem hiding this comment.
I think this will need to be added to the function that converts the units and then have it remove the aliases. We don't want these to be permanent.
|
I think some unit tests to ensure the files are read correctly would be good. |
sunpy/net/hek/hek.py
Outdated
| try: | ||
| unit = u.Unit(str.lower()) | ||
| except ValueError: | ||
| unit = u.Unit(str.capitalize()) |
There was a problem hiding this comment.
What happens if this fails?
There was a problem hiding this comment.
I have changed this function a bit. Could you please check and let me know if it's fine?
sunpy/net/hek/coord_properties.json
Outdated
| "is_unit_prop": false, | ||
| "is_chaincode": false, |
There was a problem hiding this comment.
Do we need the false values?
There was a problem hiding this comment.
Removed them and refactored the code for these changes
aritrasinha108
force-pushed
the
gsoc23-hek-improvements
branch
from
1be78cc
to
024d523
Compare
| return table | ||
|
|
||
| @staticmethod | ||
| def _parse_astropy_unit(str): |
There was a problem hiding this comment.
This function will need to be written.
| units = str.split(" per ") | ||
| unit = None | ||
| if len(units) >1: | ||
| unit = HEKClient._parse_astropy_unit(units[0]) |
There was a problem hiding this comment.
Does this method need to be part of the HEKClient?
There was a problem hiding this comment.
I wonder actually if any of these need to be attached to the client.
There was a problem hiding this comment.
where can be move them? Actually I found a utility function called _parse_times inside the client only so I wrote all the others here only.
There was a problem hiding this comment.
We move them into a seperate utils.py file? At least outside of the client itself.
| @@ -96,6 +113,125 @@ def _parse_times(table): | |||
| table[tkey].format = 'iso' | |||
| return table | |||
|
|
|||
| @staticmethod | |||
| def _parse_unit(table, attribute, is_coord_prop = False): | |||
There was a problem hiding this comment.
All of these new functions will need documentation strings and unit tests.
|
|
||
| u.add_enabled_aliases({"steradian": u.sr, "arcseconds": u.arcsec, "degrees": u.deg, "sec": u.s, "Emx": u.Mx, "Amperes": u.A, "ergs": u.erg}) |
There was a problem hiding this comment.
Won't this be better in the unit mapping below?
dstansby
added
the
Needs Adoption
|
Hello 👋, Thanks for your contribution to sunpy! |
|
Hello again 👋, We want to thank you again for your contribution to sunpy! |
PR Description
This PR is a part of the GSOC 2023 project where we intend to improve the HEK Representation in Sunpy. This specific PR integrates
astropy.unitsinto the results produced by the HEK class on making queries for solar data (anastropy.Table). This will help the end users to get a more organized and compiled form of results.TODO
astropy.coordinates.SkyCoordobjects for Chaincode type properties once the inconsistency issue is resolved.