[TEP014] Update Model and Density classes to use HDFWriter + Unit Tests #747

vg3095 · 2017-06-20T08:44:03Z

No description provided.

vg3095 · 2017-06-20T09:24:35Z

yeganer

Looks good overall but I'd suggest some changes to make implementing the tests for the other classes easier and to follow the Don't Repeat Yourself principle of python.

Another change I'd like to see which is not directly tied to this PR but could be implemented here, is changing the name of HDFWriter to HDFWriterMixin. This would then correctly reflect that HDFWriter is only a utility class and not the common Root for all our classes.

yeganer · 2017-06-20T09:48:02Z

tardis/model/base.py

 from density import HomologousDensity

 logger = logging.getLogger(__name__)


-class Radial1DModel(object):
+class Radial1DModel(HDFWriter, object):


Remove object since it is not needed here.

yeganer · 2017-06-20T09:49:33Z

tardis/model/tests/test_base.py

+
+
+@pytest.fixture(scope="module")
+def hdf_file_path(tmpdir_factory):


You should define a top level fixture in conftest.py that can be used by all test modules. That way you only have to write this code once and if it needs changes, you only have to change one instance of it.

yeganer · 2017-06-20T09:49:55Z

tardis/model/tests/test_base.py

+
+
+@pytest.fixture(scope="module")
+def actual_model():


Maybe we can have this as a top level fixture, too? See above.

yeganer · 2017-06-20T09:54:23Z

tardis/model/tests/test_base.py

+
+
+@pytest.fixture(scope="module", autouse=True)
+def to_hdf_buffer(hdf_file_path, actual_model):


This is an interesting way to achieve that the model is saved, however I'd prefer if there was a fixture that either returns the filename or the buffer the data was saved to.
Someone not very familiar with the internal workings of pytest might not understand what's happening here.

@yeganer If I return hdf buffer path from this function , then it same as input argument hdf_file_path. I think , If I return hdf_file_path here , then it is inconsistent .

yeganer · 2017-06-20T09:55:00Z

tardis/model/tests/test_density.py

+
+###
+# Save and Load
+###


See remarks for test_model.py. Most apply here aswell

yeganer · 2017-06-20T11:33:45Z

@vg3095 I haven't had the time to look in depth at the latest changes. Can you please explain, why you had to revert the name changes? I'm confused...

vg3095 · 2017-06-20T11:38:36Z

@yeganer , If I change HDFWriter to HDFWriterMixin, then I have to change it in all places in HDFWriter Unit Tests , and I think ,it will out of scope for this PR.

For now , I am importing HDFWriter in classes like this -->
from tardis.io.util import HDFWriter as HDFWriterMixin
in Radial1DModel and Homologous Density classes.

I will then, make a seperate PR for this afterwards.

yeganer

This looks good and goes into the right direction, good job!

Nevertheless I have some minor suggestions

yeganer · 2017-06-21T06:59:42Z

tardis/conftest.py

+    return model
+
+@pytest.fixture(scope="session")
+def homologous_density(hdf_config):


I would not say that homologous density is used anywhere else than the corresponding test

yeganer · 2017-06-21T07:00:24Z

tardis/conftest.py

+    return str(path)
+
+@pytest.fixture(scope="session")
+def hdf_config():


I'd suggest a rename to config_verysimple

yeganer · 2017-06-21T07:01:39Z

tardis/conftest.py

+    return config
+
+@pytest.fixture(scope="session")
+def model(hdf_config):


Can you try to create a simulation_verysimple fixture? If that works, then we can get a model by simulation_verysimple.model and therefore we don't need additional fixtures.
Do you think, that could work?

yeganer · 2017-06-21T07:09:43Z

@vg3095 You are right, the name change of HDFWriter should not be part of this PR but I think changing the name during the import is not the right way to go about this.

I'd suggest you make a small PR containing the name change and then you rebase this PR and other on your name change PR. That way you can implement it correctly right from the start but you can keep the change separate.
If you are not sure what I'm talking about in terms of git, have a look at git rebase and git rebase -i.

vg3095 · 2017-06-21T09:06:44Z

@yeganer Coverage is decreasing , due to usage of common Simulation fixture for all hdf tests, should I change it , or is it ok ?

yeganer · 2017-06-21T14:06:30Z

@vg3095 Looks good, however please remove the two commits which introduce the name change and then revert it in the next commit. You can delete, modify or merge commits with git rebase -i

yeganer · 2017-06-21T14:10:17Z

tardis/conftest.py

+
+@pytest.fixture(scope="session")
+def simulation_verysimple(config_verysimple, atomic_data_fname):
+    atomic_data = AtomData.from_hdf5(atomic_data_fname)


I know there is a problem with the atomic data and preparing it multiple times. I think we could solve this by having a session fixture that does AtomData.from_hdf(...) and another function scope fixture that takes the first fixture and returns a copy.deepcopy. Could that work?

vg3095 · 2017-06-21T15:09:36Z

@yeganer I created a separate session based atomic_dataset fixture , like you said , and in kurucz_atomic_data fixture , I used deepcopy of this session based fixture , but still the integration tests are failing. Look at the last commit.

vg3095 · 2017-06-22T04:09:32Z

The issue is fixed now.

yeganer · 2017-06-22T07:05:44Z

tardis/conftest.py

-                    '(md5="21095dd25faa1683f4c90c911a00c3f8"')
-
-    sim = Simulation.from_config(config_verysimple, atom_data=atomic_data)
+def simulation_verysimple(config_verysimple, atomic_dataset):


You are using the session scoped atomic dataset fixture here. I believe you want to use kurucz_atomic_data which is the one making use of the deepcopy, right?

@yeganer kurucz_atomic_data fixture is a function scope fixture , that`s why It can`t be used for session based Simulation fixture (It throws ScopeMismatch Error) , that`s why , I used atomic_dataset here(session based fixture) and used deepcopy to create a new copy for atomic dataset inside the function itself..

@yeganer And you are seeing the old commit

wkerzendorf · 2017-06-22T11:52:19Z

@vg3095 can you rebase onto the current master

…t.py

wkerzendorf · 2017-06-22T11:59:06Z

@yeganer and @wkerzendorf signed off - will merge when tests pass

yeganer suggested changes Jun 20, 2017

View reviewed changes

vg3095 mentioned this pull request Jun 20, 2017

[TEP014] Update Runner and Spectrum classes to use HDFWriter + Unit Tests #748

Merged

yeganer suggested changes Jun 21, 2017

View reviewed changes

vg3095 mentioned this pull request Jun 21, 2017

[TEP014] Change name of HDFWriter to HDFWriterMixin #749

Merged

vg3095 force-pushed the model_hdf branch from 51a21bb to c0e2c19 Compare June 21, 2017 08:33

yeganer reviewed Jun 21, 2017

View reviewed changes

vg3095 force-pushed the model_hdf branch from c8ec48a to cea25cf Compare June 21, 2017 14:40

yeganer reviewed Jun 22, 2017

View reviewed changes

vg3095 added 10 commits June 22, 2017 17:23

Update Radial1DModel class to use HDFWriter

95ccbbe

Update Homologous Density class to use HDFWriter

565e329

Added hdf tests for Radial1DModel class

d153e5e

Added hdf tests for Homologous Density class

d1cb147

Move common hdf fixtures to conftest.py

5567e2d

Fix import of HDFWriterMixin in Model and Density Classes

86d3b32

Create a common Simulation fixture under conftest.py for HDF tests

0524d8e

Fix for atomic_dataset used to create common Simulation hdf fixture

f0ec102

Fix for atomic_data fixture in conftest.py

ca67fb0

Use deepcopy for atomic_dataset in Simultation hdf fixture in conftes…

d1074a1

…t.py

vg3095 force-pushed the model_hdf branch from 01a0eb6 to d1074a1 Compare June 22, 2017 11:55

wkerzendorf merged commit 262c1a3 into tardis-sn:master Jun 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP014] Update Model and Density classes to use HDFWriter + Unit Tests #747

[TEP014] Update Model and Density classes to use HDFWriter + Unit Tests #747

vg3095 commented Jun 20, 2017

vg3095 commented Jun 20, 2017

yeganer left a comment

yeganer Jun 20, 2017

yeganer Jun 20, 2017

yeganer Jun 20, 2017

yeganer Jun 20, 2017

vg3095 Jun 20, 2017

yeganer Jun 20, 2017

yeganer commented Jun 20, 2017

vg3095 commented Jun 20, 2017

yeganer left a comment

yeganer Jun 21, 2017

yeganer Jun 21, 2017

yeganer Jun 21, 2017

yeganer commented Jun 21, 2017

vg3095 commented Jun 21, 2017 •

edited

Loading

yeganer commented Jun 21, 2017

yeganer Jun 21, 2017

vg3095 commented Jun 21, 2017

vg3095 commented Jun 22, 2017

yeganer Jun 22, 2017

vg3095 Jun 22, 2017 •

edited

Loading

vg3095 Jun 22, 2017

wkerzendorf commented Jun 22, 2017

wkerzendorf commented Jun 22, 2017



		@pytest.fixture(scope="module")
		def hdf_file_path(tmpdir_factory):



		@pytest.fixture(scope="module", autouse=True)
		def to_hdf_buffer(hdf_file_path, actual_model):

[TEP014] Update Model and Density classes to use HDFWriter + Unit Tests #747

[TEP014] Update Model and Density classes to use HDFWriter + Unit Tests #747

Conversation

vg3095 commented Jun 20, 2017

vg3095 commented Jun 20, 2017

yeganer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeganer commented Jun 20, 2017

vg3095 commented Jun 20, 2017

yeganer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeganer commented Jun 21, 2017

vg3095 commented Jun 21, 2017 • edited Loading

yeganer commented Jun 21, 2017

Choose a reason for hiding this comment

vg3095 commented Jun 21, 2017

vg3095 commented Jun 22, 2017

Choose a reason for hiding this comment

vg3095 Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkerzendorf commented Jun 22, 2017

wkerzendorf commented Jun 22, 2017

vg3095 commented Jun 21, 2017 •

edited

Loading

vg3095 Jun 22, 2017 •

edited

Loading