Improving HEK Representation #7619
Conversation
|
You will need to add regions to https://github.com/sunpy/sunpy/blob/main/pyproject.toml#L77 so it is installed. |
sunpy/net/hek/utils.py
Outdated
| with u.add_enabled_units([cm2, m2, m3]), u.set_enabled_aliases(aliases): | ||
| if is_coord_prop: | ||
| coord1_unit, coord2_unit, coord3_unit = None, None, None | ||
| coord_units = re.split(r'[, ]', str) |
There was a problem hiding this comment.
Do you know what units would split here?
There was a problem hiding this comment.
The coordination units, right?
For example arcsec,arcsec
There was a problem hiding this comment.
Ah I see. Is that the only one or are there more?
There was a problem hiding this comment.
I'm not sure, but this is the only example I saw in the json files. Maybe another usage would be (X, Y, Z)
There was a problem hiding this comment.
Can you run the code and see how many and what units trigger this?
|
Looks like there is a merge conflict with sunpy/main now. Can you merge that in and resolve that? |
| @@ -12,6 +17,23 @@ | |||
| def foostrwrap(request): | |||
| return hek.attrs._StringParamAttrWrapper("foo") | |||
|
|
|||
| @pytest.fixture | |||
| def read_unit_attributes(): | |||
| UNIT_FILE_PATH = Path(os.path.dirname(__file__)) / "../unit_properties.json" | |||
There was a problem hiding this comment.
Is there another way to get this file path without having to add the ".."?
There was a problem hiding this comment.
Can we not import this from the utils file?
| elif attribute["frame"] == "heliocentric": | ||
| coord1_unit = u.R_sun | ||
| coord2_unit = u.deg | ||
| elif attribute["frame"] == "icrs": |
There was a problem hiding this comment.
Can you tell me which attributes give an icrs value?
There was a problem hiding this comment.
Bound_ChainCode and Skel_ChainCode
There was a problem hiding this comment.
Can you start a document or make a note of this? I want to make sure we capture any of this quirks but not via code comments.
sunpy/net/hek/utils.py
Outdated
| coord1_unit = u.Unit(coord_units[0]) | ||
| coord2_unit = u.Unit(coord_units[1]) | ||
| coord3_unit = u.Unit(coord_units[2]) | ||
| return locals()[unit_prop] |
There was a problem hiding this comment.
I think the three units should be the same, so we can just do this:
if unit_prop in ["coord1_unit", "coord2_unit", "coord3_unit", "event_coordunit"]:
coord_units = re.split(r'[, ]', unit)
return u.Unit(coord_units[0])There was a problem hiding this comment.
Can you try that and see how it goes?
| UNIT_FILE_PATH = Path(os.path.dirname(__file__)) / "unit_properties.json" | ||
| COORD_FILE_PATH = Path(os.path.dirname(__file__)) / "coord_properties.json" |
There was a problem hiding this comment.
I think these can be added to the all and imported else where as required.
| COORD_FILE_PATH = Path(os.path.dirname(__file__)) / "coord_properties.json" | ||
|
|
||
| def freeze(obj): | ||
| """ Create hashable representation of result dict. """ |
There was a problem hiding this comment.
| """ Create hashable representation of result dict. """ | |
| """ | |
| Create a hashable representation of a dict or list. | |
| Otherwise returns the original input. | |
| """ |
There was a problem hiding this comment.
I take issue with the function name but that's a separate conversation.
This PR includes the work that has been done in GSoC 23 but in a new and fresh branch to start GSoC 24.
I've migrated GSoC 23 commits and resolved most of the conflicts, it needs a review to see if there is any conflict remaining.