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

properly save and retrieve a SkyCoord object when it's a scalar or N-D array #136

Merged
merged 3 commits into from
Jul 28, 2020

Conversation

r-xue
Copy link
Contributor

@r-xue r-xue commented Jul 1, 2020

The current implementation always saves the SkyCoord object as an Nx2 array and doesn't work well if the coordinate value is a scalar or high-dimensional array

In [2]: radec = SkyCoord(0, 0, unit='degree') 
   ...: hkl.dump(radec, "test_ap.h5") 
   ...: radec2 = hkl.load("test_ap.h5") 
   ...: print(radec,radec.isscalar) 
   ...: print(radec2,radec2.isscalar)                                                                                                                                           
<SkyCoord (ICRS): (ra, dec) in deg
    (0., 0.)> True
<SkyCoord (ICRS): (ra, dec) in deg
    [(0., 0.)]> False

In [3]: radec = SkyCoord(np.zeros((3,4)), np.zeros((3,4)), unit='degree') 
   ...: hkl.dump(radec, "test_ap.h5") 
   ...: radec2 = hkl.load("test_ap.h5") 
   ...: print(radec.shape) 
   ...: print(radec2.shape)                                                                                                                                                    
(3, 4)
(3,)

This PR is supposed to fix the issue. test_astropy.py is also slightly updated to reflect the change.

@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #136 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #136   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          576       576           
=========================================
  Hits           576       576           
Impacted Files Coverage Δ
hickle/loaders/load_astropy.py 100.00% <100.00%> (ø)
hickle/loaders/load_numpy.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e054839...ef02def. Read the comment docs.

@1313e
Copy link
Collaborator

1313e commented Jul 1, 2020

Huh, surprising that I did not catch that SkyCoord has no array equivalent test, which would have caught this.
Nice catch!

Can you make a PR with the appropirate changes to the v3 branch, and remove the changes you made to legacy_v3 in this PR?
That way, we can fix this in both v3 and v4 (and you get credit for the fix in both versions).

@r-xue
Copy link
Contributor Author

r-xue commented Jul 1, 2020

okay. it's done. hopefully, I get everything right.

@r-xue
Copy link
Contributor Author

r-xue commented Jul 1, 2020

add another small change related to #37 (which shows up again in v4). the v3 branch seems to be fine.
not relevant to the initial PR purpose, but let's get it covered here?

@1313e
Copy link
Collaborator

1313e commented Jul 28, 2020

@r-xue Thanks for that.
I finally have time again to do more important stuff, like this.

The last commit you added will be reverted, as it not only does not belong to this PR, but also because I very specifically reintroduced that change in v4.
Compression isn't the only thing kwargs can have.
A user would expect them to be passed along, so that's what I did.
As you normally cannot compress scalars in HDF5, I see no reason why the user would expect that to be a thing when using hickle.
And if HDF5 does allow compression on scalars in the future (doubtful, but possible), it would all of a sudden require an additional change in hickle to allow for that.

So, for that reason, I simply let h5py and HDF5 handle that error.
In case you are a bit uncomfortable with that: I did add something else to v4, where if any error is raised anywhere during dumping, it will dump the contents first using pickle and only reraise the error afterward.
You will therefore not lose your data due to attempting to store something incorrectly.

1313e added a commit that referenced this pull request Jul 28, 2020
the same fix from PR #136 to v3
@1313e 1313e changed the base branch from master to dev July 28, 2020 07:00
@1313e 1313e merged commit 2be935f into telegraphic:dev Jul 28, 2020
@r-xue
Copy link
Contributor Author

r-xue commented Jul 28, 2020

yeah. I agree. keep consistency is important. Thank you for the excellent work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants