simphony / simphony-common

The native implementation of the Simphony cuds objects
BSD 2-Clause "Simplified" License
7 stars 5 forks source link

PR: Feature choose which CUBA keys are stored in H5CUDS file #244

Closed tuopuu closed 8 years ago

tuopuu commented 8 years ago

This PR adds an optional cuba_keys argument to H5CUDS.add_dataset which can be used to select which CUBA keys are stored in the H5CUDS file. This PR addresses #229.

tuopuu commented 8 years ago

There was some duplicate testing in test_h5_cuds for mesh and particles datasets, which is already covered in abc_check_engine template. I think we can remove these test.

nathanfranklin commented 8 years ago

There was some duplicate testing in test_h5_cuds for mesh and particles datasets, which is already covered in abc_check_engine template. I think we can remove these test.

It looks like there is lots of overlap with abc_check_engine but a few things appear to be be unique. Like for example https://github.com/simphony/simphony-common/blob/master/simphony/io/tests/test_h5_cuds.py#L90 , https://github.com/simphony/simphony-common/blob/master/simphony/io/tests/test_h5_cuds.py#L151-L154

Specifically, https://github.com/simphony/simphony-common/blob/master/simphony/io/tests/test_h5_cuds.py#L151-L154 where its the only place we test that we can't use the (i) H5Cuds file or (ii) any dataset proxy handles that were gotten from that file after the file been closed. That said, it looks like this only performs the (ii) test for meshes.

tuopuu commented 8 years ago

(i) H5Cuds file or (ii) any dataset proxy handles that were gotten from that file after the file been closed.

I don't think the engine test template actually tests this either for any engine: are the handles usable after the engine has been closed/deleted? I think it could be added to the engine template and by that used everywhere.

tuopuu commented 8 years ago

Reopening the file (even though it's kind of analogous to reconnecting/recreating the handle to a modeling engine) should be tested here, but for it we can write its own test.

nathanfranklin commented 8 years ago

are the handles usable after the engine has been closed/deleted?

We don't have 'close' in ABCModelingEngine

(i) H5Cuds file or (ii) any dataset proxy handles that were gotten from that file after the file been closed.

I don't think the engine test template actually tests this either for any engine: are the handles usable after the engine has been closed/deleted? I think it could be added to the engine template and by that used everywhere.

My initial feeling is that we should not alter the abc_check_engine for this PR. H5Cuds has lots of overlap with ABCModelingEngine (all the 'XXX_dataset' methods) and so uses the testing framework for the engine (abc_check_engine) but H5Cuds is different beast. And has some unique behavior (e.g. files can be closed). It also has some new differences to ABCModelingEngine that are added to this PR (i.e. 'add_dataset' has optional parameter) which need to be tested.

tuopuu commented 8 years ago

My initial feeling is that we should not alter the abc_check_engine

Yeah, it's not good make changes here. We'll have another issue for it if there's a need. I'll just clean the test_h5_cuds file a bit from the overlapping parts, and add the missing tests to the cuds-file handle related class. I'll make a push in a minute, so please take a look at it then. If there's something you'd still like to have there, let me know.

tuopuu commented 8 years ago

Testing that closed file has the data still in it, I'll add it.

edt. not missing but existing in it

nathanfranklin commented 8 years ago

:+1: Looks good to me. If someone else has time, maybe someone else should take a look too before merging.

tuopuu commented 8 years ago

Thanks @nathanfranklin, I think we can merge this. (No objections in a week.)