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

Eliminate compatibility mode from HDF5 I/O #8899

Merged

Conversation

matteobachetti
Copy link
Contributor

@matteobachetti matteobachetti commented Jun 21, 2019

As per #8888

  • Eliminate compatibility mode writing
  • Changelog etc.

@taldcroft, @pllim, @astrofrog, @bsipocz et al.: is there a use case for being able to still read the old format?

@pllim pllim added this to the v4.0 milestone Jun 21, 2019
@pllim
Copy link
Member

pllim commented Jun 21, 2019

Thanks! I don't use HDF5, so I cannot comment. 😬

@bsipocz
Copy link
Member

bsipocz commented Jun 21, 2019

Please remove changes to the helpers.

I don't use hdf5 files so don't know how widely the old formats are used, and what the actual issue with it. Naively I would say we should be able to read in as many types of files as possible, but should not write non standard or obsolete versions of them.

@matteobachetti matteobachetti force-pushed the eliminate_hdf5_compatibility_mode branch from 43f4881 to bc512e8 Compare June 21, 2019 16:55
@matteobachetti
Copy link
Contributor Author

Oh, sorry for the helpers thing. Done

@matteobachetti matteobachetti force-pushed the eliminate_hdf5_compatibility_mode branch from bc512e8 to 86ffc83 Compare June 21, 2019 17:00
@matteobachetti
Copy link
Contributor Author

@bsipocz the main problem I see is that the old reader now remains untested because we have no way to write old-format files. Besides that, I would also prefer to keep as many formats as possible

@pllim
Copy link
Member

pllim commented Jun 21, 2019

Re: testing old format reader -- Is it possible to write it out now while we can and store it as a test data file (either remote or local depending on file size and your philosophy about data files).

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with removing this, and also agree with @pllim's suggestion - we should basically take the example from the first half of test_preserve_serialized_compatibility_mode, write it out as a file, store it with the data files for tests (inside a data directory), and use it to continue to test the reading. I think the file will be very small, so I'm not worried about it, and we shouldn't have to change it in future since it's just a legacy format.

@astrofrog
Copy link
Member

@matteobachetti - could you rebase, to get rid of the merge commits?

@matteobachetti matteobachetti force-pushed the eliminate_hdf5_compatibility_mode branch 2 times, most recently from b69778e to 50add74 Compare June 29, 2019 16:58
@matteobachetti
Copy link
Contributor Author

@astrofrog: done, but 32-bit tests are falling and I'm not sure why

@matteobachetti matteobachetti changed the title [WIP] Eliminate compatibility mode from HDF5 I/O Eliminate compatibility mode from HDF5 I/O Jul 1, 2019
@matteobachetti
Copy link
Contributor Author

Oh, I see the likely problem with the 32-bit build: #8934

@bsipocz
Copy link
Member

bsipocz commented Jul 2, 2019

@matteobachetti - yes, failures should be unrelated. Given that this needs a rebase anyway due to the unrelated commit, I would say rebase after #8945 (or alternative) is merged.

@matteobachetti
Copy link
Contributor Author

Oh, I wonder why that commit is there at all. I already rebased in theory O_o.
Anyway, the plan sounds good. Waiting for solution for 32 bits, then rebase. Thanks!

@bsipocz
Copy link
Member

bsipocz commented Jul 2, 2019

@matteobachetti - the workaround in merged, so you can rebase. You can manually remove the commit if you rebase in interactive mode.

@matteobachetti matteobachetti force-pushed the eliminate_hdf5_compatibility_mode branch from 50add74 to 718883c Compare July 2, 2019 00:48
@matteobachetti
Copy link
Contributor Author

@bsipocz rebased and dropped commit.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review comments have been addressed.

@bsipocz bsipocz dismissed astrofrog’s stale review July 2, 2019 02:44

review has been addressed

@bsipocz bsipocz merged commit f4bf28e into astropy:master Jul 2, 2019
@bsipocz
Copy link
Member

bsipocz commented Jul 2, 2019

Thanks @matteobachetti

@matteobachetti matteobachetti deleted the eliminate_hdf5_compatibility_mode branch July 2, 2019 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants