Integration Test class for Stratified W7 setup. #558

kdexd · 2016-05-09T03:07:18Z

This PR focuses on addition and proper functioning of an integration test class for Stratified W7 setup. This test is classified as a slow test due to its time taken for a run on single core.

Important components added:

Configuration file: tardis/tests/tests_slow/w7/config_w7.yml
Abundances profile: tardis/tests/tests_slow/w7/abundances_w7.dat
Densities profile: tardis/tests/tests_slow/w7/densities_w7.dat
Integration test class: tardis/tests/tests_slow/test_w7.py

Other relevant documentation is added in Integration test itself.

kdexd · 2016-05-09T03:09:40Z

tardis/tests/tests_slow/test_w7.py

+        * ws                             |     * j_blues_norm_factor
+
+    Also assumed `kurucz_cd23_chianti_H_He.h5` file exists in `tmp` directory.
+    """


Docstring for explanation related to work in progress.

wkerzendorf · 2016-05-09T15:42:53Z

you need to rebase this commit on the other one.

wkerzendorf · 2016-05-09T15:43:29Z

tardis/tests/tests_slow/test_w7.py

+
+@pytest.mark.skipif(not pytest.config.getoption("--with-slow"),
+                    reason="slow tests can only be run using --with-slow")
+class TestW7(object):


This can likely be subclassed.

- --baseline-data argument will acept the path of directory containing the baseline data for assertion. - Fix --with-slow flag, and replace it with --slow, after a rebase on the base branch.

kdexd · 2016-05-10T06:43:42Z

@wkerzendorf @yeganer @unoebauer I am experiencing an IOError everytime now, and this test doesn't work. The problem is that tests_slow directoy and its contents are not included the the directory tree created after the build. This problem was not faced earlier while I implemented it pre-GSoC in #508. Infact, the test was run properly and I have reported it in one of the comments. Is there any recent change in build pattern which I am unaware of ?

I had experimentally let this slow test run on Travis of my fork, whether to make sure that my loca copy itself is not faulty. Here is the build job about that. I am facing the same error locally.

wkerzendorf · 2016-05-10T07:05:00Z

tardis/tests/tests_slow/test_w7.py

+        * nubar_estimators               |     * last_line_interaction_angstrom
+        * ws                             |     * j_blues_norm_factor
+
+    Also assumed `kurucz_cd23_chianti_H_He.h5` file exists in `tmp` directory.


It needs an atomic dataset. Once the test gets running, I will update this. Atomic dataset will be provided by user through command line (before this PR gets merged). It is actually too long to type on command line 😅

you should use the existing infrastructure to pass the atomic datafile

Sure, I will do it same as test_tardis_full.py

wkerzendorf · 2016-05-10T08:45:35Z

your errors are related to not including these files in a setup_package.py - have a look throughout TARDIS for this file and it shows you how you need to include your files.

- This makes sure that they are included in the directoy structure after the build is executed.

wkerzendorf · 2016-05-10T13:32:22Z

tardis/tests/tests_slow/test_w7.py

+                    reason="slow tests can only be run using --slow")
+@pytest.mark.skipif(not pytest.config.getvalue("baseline-data"),
+                    reason="--baseline-data was not specified")
+class TestW7:


inherit from object to get new-style classes.

- Make Testw7 class inherit from 'object' for new style classes. - Replace %s style string formatting with .format()

wkerzendorf · 2016-05-11T06:38:17Z

@yeganer I'm signing off on this.

unoebauer · 2016-05-11T07:56:27Z

tardis/tests/tests_slow/test_w7.py

+
+        # First we check whether atom data file exists at desired path.
+        self.atom_data_filename = os.path.expanduser(os.path.expandvars(
+                                    pytest.config.getvalue('atomic-dataset')))


We may want to add a check that the atomic database is the same as used for the baseline data. Otherwise, this test most likely won't pass. Not sure if we have to include this check in the current PR, but something to keep in mind, though. Thoughts, @wkerzendorf, @karandesai-96, @yeganer?

This didn't even come to my mind ! Using a different atomic database would generate different results. So far there is kurucz_cd23_chianti_H_He.h5 everywhere. If this check is added when need arrives, then the purpose of adding that additional check would be more clear and could be specified in PR description more effectively. Waiting for others' opinions 😄

can you just check the uuid of the atomic data set - that would be good.

unoebauer · 2016-05-11T07:59:18Z

Apart from my inline comment concerning atomic data integrity, I am very happy with the PR. As soon as we've come to a decision concerning this point, we can merge.

- It is to be made sure that baseline data was obtained using the same atomic dataset as the current test run. Hence both UUID must be equal.

unoebauer · 2016-05-11T12:47:55Z

tardis/tests/tests_slow/test_w7.py

+        # the same atomic dataset as the one used in current test run.
+        # TODO: This is a quick workaround, generalize it.
+
+        kurucz_data_file_uuid = "5ca3035ca8b311e3bb684437e69d75d7"


Not too happy about this quick workaround. You can directly access the UUID and the MD5 checksum of the atomic data file via model.atom_data.uuid1 and model.atom_data.md5. I suggest you add these to your baseline data and then check them against the one of the test run.

Oh, so after the run, I obtain the radial1d_model and that would enable comparison, right ? But wouldn't it be a bit time expensive ? Waiting for twenty minutes or more and finally seeing that the test failed because atom data files weren't same ? Is it fine ?

I suggest you add these to your baseline data and then check them against the one of the test run.

Add them in the sense, like a json or yml file in the directory containing baseline data for slow tests, right ?

atom_data: name: kurucz_cd23_chianti_H_He.h5 uuid1: <uuid1> md5: <md5>

I think this would conflict with --atomic-dataset argument. In that case we can give the yml file a priority and NOT skip this test if --atomic-dataset is explicitly not provided. Does it sound like a plan ?
@wkerzendorf @yeganer Your thoughts ?

@unoebauer I suggested this quick workaround. I agree that in the long run we want to have this done properly, but my suggestion is to build parts of the framework and then refine. Would that work for you?

@wkerzendorf, @karandesai-96 - sure thing. Sorry for holding up the process. Just wanted to flag this again.

Okay if we not do apply this change, though please do let me know about this idea of putting in a yml file like this. If it is good enough, I would keep it in my mind and apply it immediately when required 😉

You don't have to wait for the run to be completed to access AtomData.
Afaik Atomdata is already an object inside tardis_config(the parsed, validated one) so doing something like tardis_config.atom_data.uuid should work.

About storing the reference: I think hardcoding it into the tests is fine for now, as there won't be any other database in the near future and this is just a simple implementation detail which can be changed anytime in the future.

config_yaml = yaml.load(open(self.config_file)) config_yaml['atom_data'] = self.atom_data_filename

We open the config file and do an overwrite with the new path of atom data filename (the one which we provided from command line). It doesn't seem like an object. Did I miss anything ?

Oh sorry, it was a misunderstanding, @yeganer I did as you suggested and it works fine. :)

unoebauer · 2016-05-11T13:17:34Z

Happy to merge if nobody objects...

yeganer · 2016-05-11T13:53:32Z

tardis/tests/tests_slow/test_w7.py

+        config_yaml['model']['structure']['filename'] = self.densities
+
+        # The config hence obtained will be having appropriate file paths.
+        tardis_config = Configuration.from_config_dict(config_yaml)


That's where the config get's validated and where atom_data becomes an object.

If I'm wrong, you can "load" the atomdata manually and pass the object to the config or the model. IIRC they all accept either a string or an AtomData object.
This might be easier anyway, instead of manipulating the config, load the AtomData directly.

This strategy sounds better. I am trying it.

wkerzendorf · 2016-05-11T13:56:04Z

@yeganer if you do not object - I'm merging now.

yeganer · 2016-05-11T13:57:00Z

@wkerzendorf I'm objecting for now.

yeganer · 2016-05-11T13:58:26Z

tardis/tests/tests_slow/test_w7.py

+        # the same atomic dataset as the one used in current test run.
+        with h5py.File(self.atom_data_filename, 'r') as slow_test_atom_data_file:
+            slow_test_data_file_uuid1 = slow_test_atom_data_file.attrs['uuid1']
+        assert slow_test_data_file_uuid1 == tardis_config.atom_data.uuid1


this translates to assert a == a
compare tardis_config with your hardcoded uuid

and - that is sensible. if someone uses a different atomic dataset with the same name it will fail.

yeganer · 2016-05-12T08:10:11Z

Looks good now.

kdexd reviewed May 9, 2016
View reviewed changes

wkerzendorf reviewed May 9, 2016
View reviewed changes

karandesai-96 added 6 commits May 10, 2016 07:42

Add config and data files of Stratified W7 setup.

ff973b0

Omit slow tests from coverage.py's html report.

fda44a9

Add setup method for W7 integration test.

a5eb242

Rearrange directory structure.

ff504e0

Add all methods for performing assertions in TestW7.

1fcf6f0

Add command line arg to provide baseline data directory.

48c3622

- --baseline-data argument will acept the path of directory containing the baseline data for assertion. - Fix --with-slow flag, and replace it with --slow, after a rebase on the base branch.

kdexd force-pushed the w7-integration-test-class branch from 9d415f0 to 48c3622 Compare May 10, 2016 02:49

wkerzendorf reviewed May 10, 2016
View reviewed changes

Karan Desai added 2 commits May 10, 2016 16:31

Accept atom data file path by command line args

57d6ca4

Add tests_slow directory to setup_package.py

e23319b

- This makes sure that they are included in the directoy structure after the build is executed.

wkerzendorf reviewed May 10, 2016
View reviewed changes

Minor cosmetic changes.

d665b9e

- Make Testw7 class inherit from 'object' for new style classes. - Replace %s style string formatting with .format()

kdexd mentioned this pull request May 10, 2016

GSoC '16 Testing Project, prelim task: skip slow tests (not to be merged) #508

Closed

kdexd force-pushed the w7-integration-test-class branch from 1d5fa2e to d665b9e Compare May 10, 2016 16:06

unoebauer reviewed May 11, 2016
View reviewed changes

Karan Desai added 2 commits May 11, 2016 18:04

Add comparison of UUIDs of atom data files.

8e3bbac

- It is to be made sure that baseline data was obtained using the same atomic dataset as the current test run. Hence both UUID must be equal.

Rename '--baseline-data' flag as '--slow-test-data'

1119315

unoebauer reviewed May 11, 2016
View reviewed changes

yeganer self-assigned this May 11, 2016

yeganer added ready and removed ready labels May 11, 2016

yeganer reviewed May 11, 2016
View reviewed changes

Refactor the assertion of atom data file uuid1.

815937b

yeganer reviewed May 11, 2016
View reviewed changes

Give atom data separately to config instead of filepath.

f952367

wkerzendorf merged commit d8eeb9d into tardis-sn:master May 12, 2016

wkerzendorf removed the in progress label May 12, 2016

kdexd deleted the w7-integration-test-class branch May 12, 2016 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration Test class for Stratified W7 setup. #558

Integration Test class for Stratified W7 setup. #558

kdexd commented May 9, 2016 •

edited by yeganer

Loading

kdexd May 9, 2016

wkerzendorf commented May 9, 2016

wkerzendorf May 9, 2016

kdexd commented May 10, 2016

wkerzendorf May 10, 2016

kdexd May 10, 2016

wkerzendorf May 10, 2016

kdexd May 10, 2016

wkerzendorf commented May 10, 2016

wkerzendorf May 10, 2016

wkerzendorf commented May 11, 2016

unoebauer May 11, 2016

kdexd May 11, 2016

wkerzendorf May 11, 2016

unoebauer commented May 11, 2016

unoebauer May 11, 2016 •

edited

Loading

kdexd May 11, 2016

kdexd May 11, 2016 •

edited

Loading

wkerzendorf May 11, 2016

unoebauer May 11, 2016

kdexd May 11, 2016

yeganer May 11, 2016 •

edited

Loading

kdexd May 11, 2016 •

edited

Loading

kdexd May 11, 2016

unoebauer commented May 11, 2016

yeganer May 11, 2016 •

edited

Loading

kdexd May 11, 2016

wkerzendorf commented May 11, 2016

yeganer commented May 11, 2016 •

edited

Loading

yeganer May 11, 2016 •

edited

Loading

wkerzendorf May 11, 2016

yeganer commented May 12, 2016

Integration Test class for Stratified W7 setup. #558

Integration Test class for Stratified W7 setup. #558

Conversation

kdexd commented May 9, 2016 • edited by yeganer Loading

Choose a reason for hiding this comment

wkerzendorf commented May 9, 2016

Choose a reason for hiding this comment

kdexd commented May 10, 2016

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

wkerzendorf commented May 10, 2016

Choose a reason for hiding this comment

wkerzendorf commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unoebauer commented May 11, 2016

unoebauer May 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdexd May 11, 2016 • edited Loading

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 May 11, 2016 • edited Loading

Choose a reason for hiding this comment

kdexd May 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unoebauer commented May 11, 2016

yeganer May 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkerzendorf commented May 11, 2016

yeganer commented May 11, 2016 • edited Loading

yeganer May 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeganer commented May 12, 2016

kdexd commented May 9, 2016 •

edited by yeganer

Loading

unoebauer May 11, 2016 •

edited

Loading

kdexd May 11, 2016 •

edited

Loading

yeganer May 11, 2016 •

edited

Loading

kdexd May 11, 2016 •

edited

Loading

yeganer May 11, 2016 •

edited

Loading

yeganer commented May 11, 2016 •

edited

Loading

yeganer May 11, 2016 •

edited

Loading