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

File IO: clean up and test #498

Merged
merged 10 commits into from
Jan 20, 2015
Merged

File IO: clean up and test #498

merged 10 commits into from
Jan 20, 2015

Conversation

ketch
Copy link
Member

@ketch ketch commented Jan 15, 2015

Summary

This PR adds tests for the ASCII and HDF5 file IO routines, in which a solution is read from a binary file and an HDF5 file, written to ASCII and HDF5 files, and then read in again from those files. The resulting solution objects are then checked for equality.

Additionally, this PR refactors most of the IO routines, including the following changes:

  • Files are opened using "with" statements, so that they get closed even if an exception is thrown (in one case, the previous code did not close files at all)
  • other bits of code are handled more pythonically
  • undesirable things like "exec" have been removed

Additional notes

Going through this highlighted to me that we are writing different sets of attributes in different cases. It seems worthwhile to define precisely the required and optional attributes of

  • Solution
  • State
  • Patch
  • Grid
  • Dimension

Finally, the code in io_test.py could be simplified by defining an eq operator for these classes, which would also encode the information about required and optional attributes.

Use with statements so that files are always closed.
Remove special error handling for IO errors, since default
traceback is just as informative.
Write NaNs to aux array if it is not initialized.
- always close files
- no special IO error handling, since default traceback is just as
  informative
- combine dimension-independent parts of code
Always close files.
Simpler handling of default options.
- always close files
- move logger message that could never be reached
- remove exceptions that could never be reached
- remove options code that does nothing in read function
- remove bc_lower and bc_upper which no longer belong to dimension
  objects
- don't use exec
- remove comments that merely restate clear code
examples/euler_3d/test_io.py tests the ASCII and HDF5 file formats.
@mandli
Copy link
Member

mandli commented Jan 16, 2015

Can we add the hdf5 module to the travis testing?

@@ -0,0 +1,61 @@
import os
Copy link
Member

Choose a reason for hiding this comment

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

This all looks good. Is the plan to leave this test in examples?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't worried about where to put it, since nose picks up everything. Currently we run nose from the examples directory, though.

@ahmadia
Copy link
Member

ahmadia commented Jan 16, 2015

👍

@ketch
Copy link
Member Author

ketch commented Jan 16, 2015

Can we add the hdf5 module to the travis testing?

Dang, I forgot that we don't have that installed on Travis. I think the problem is that the install times out if we add more things. Maybe we should try miniconda:

http://conda.pydata.org/docs/travis.html
https://gist.github.com/dan-blanchard/7045057

@mandli
Copy link
Member

mandli commented Jan 16, 2015

@ahmadia I hate to bother you about this yet again but is there any chance we could go with a <#> build or would it still be too slow without binary builds?

@ahmadia
Copy link
Member

ahmadia commented Jan 16, 2015

Actually, @cekees came up with a clever method for working around hashdist's binary incompatibility. He's implemented his approach on Shippable, but I should be able to get it going on Travis CI. I can take a swing on it tomorrow. The main idea is to take advantage of the fact that the Travis build will always be of the form: /home/travis and we can prebuild for that operating system.

@mandli
Copy link
Member

mandli commented Jan 16, 2015

Ah nice! It will be great to have this issue crossed off our list of problems.

@cekees
Copy link

cekees commented Jan 16, 2015

Yeah, all you need to do is find a way to run the image travis or
shippable is using, then tar up your .hashdist directory and post it
somewhere public. Been meaning to post the approach on the hashstack wiki
or something. The proteus Shippable CI is down right now because
proteus.usace.army.mil is down, and that's where I had been stashing
hashdist.tgz, but you can take a look at the shippable.yaml file at
http://github.com/erdc-cm/proteus to see how it works.

On Fri, Jan 16, 2015 at 10:17 AM, Kyle Mandli notifications@github.com
wrote:

Ah nice! It will be great to have this issue crossed off our list of
problems.


Reply to this email directly or view it on GitHub
#498 (comment).

@ahmadia
Copy link
Member

ahmadia commented Jan 16, 2015

I'm building the Travis Python VM on my laptop now. I'll play with putting together a hashed stack together. This would be useful to a number of projects with complex builds.

@rjleveque
Copy link
Member

@ketch, I'll send you a binary file for testing.

@ketch
Copy link
Member Author

ketch commented Jan 17, 2015

Thanks @rjleveque . I have corrected binary.py and it reads just fine now. Further testing reveals that the hdf5 writer is not correct for AMR files (as I expected) so I'm working on that now.

@ketch
Copy link
Member Author

ketch commented Jan 17, 2015

I will add tests for reading binary and writing AMR HDF5 soon.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f778c63 on ketch:more_io_cleanup into 81166c7 on clawpack:master.

@ahmadia
Copy link
Member

ahmadia commented Jan 18, 2015

I added h5py in #499.

Also moved IO tests to pyclaw/tests/ directory.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ee6d76e on ketch:more_io_cleanup into 81166c7 on clawpack:master.

Pass name as keyword argument.
* master:
  Add ability to skip Sedov test (disable PETSc)
  Need pip in stack python for coverage/coveralls
  Move coverage tools to hashstack build
  Add scipy/mpl/h5py, streamline Travis
  Remove verbosity from untar
  Fast binary-cached build of hashdist dependencies
In order to pick up new IO tests.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 940ec70 on ketch:more_io_cleanup into 09cd14f on clawpack:master.

@ketch
Copy link
Member Author

ketch commented Jan 20, 2015

This is now ready to merge. As a bonus, some coverage is being reported again.

mandli added a commit that referenced this pull request Jan 20, 2015
@mandli mandli merged commit 44e3ec7 into clawpack:master Jan 20, 2015
@ketch ketch deleted the more_io_cleanup branch January 20, 2015 17:34
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.

None yet

6 participants