Replace Comparison values in Plasma Unit Tests with reference HDF file #774

vg3095 · 2017-08-03T06:11:33Z

This PR contains following changes -

Compare values from a reference HDF file , rather than from hard coded values

Some new tests for Plasma

General Properties
- Dilution factor('w')
Atomic Properties
- nu
- wavelength_cm
- f_lu
- metastability
Radiative Properties
- transition_probabilites
Scalar Properties
- helium_treatment
- link_t_rad_t_electron
- time_explosion
NTLE
- Ion Population Properties
  - previous_electron_densities
- Radiative Properties
  - previous_beta_sobolev

vg3095 · 2017-08-03T06:25:14Z

I have currently replaced values for test_partition_function.py. If my approach is correct, then I will expand it to other plasma tests.

vg3095 · 2017-08-03T06:26:25Z

@wkerzendorf Can you check my approach ?

vg3095 · 2017-08-07T06:48:35Z

.travis.yml

        - MINICONDA_URL='http://repo.continuum.io/miniconda/Miniconda-latest-Linux-x86_64.sh'

 matrix:
    include:
        - python: 2.7
          env:
            - COMPILER=gcc
-            - SETUP_CMD='test --args="--atomic-dataset=$HOME/kurucz_cd23_chianti_H_He.h5"'
+            - SETUP_CMD='test --args="--atomic-dataset=$HOME/kurucz_cd23_chianti_H_He.h5 --plasma-reference=$HOME/plasma_reference/"'


@wkerzendorf Here I want to pass path for plasma-reference files, but travis doesn't recognise this option.

I have added this option in conftest.py and this option works fine in my laptop.
So, what should I do here?

@yeganer has some comments on this - I let him discuss this with you.

wkerzendorf · 2017-08-07T08:29:09Z

.travis.yml

@@ -21,27 +21,29 @@ env:
        - SETUP_CMD='test'
        - TEST_MODE='normal'
        - ATOM_DATA_URL='https://github.com/tardis-sn/tardis-atomdata/raw/master/kurucz_cd23_chianti_H_He.h5.zip'
+        - PLASMA_NLTE_REFERENCE_URL='https://github.com/vg3095/tardis-refdata/raw/plasma-ref2/plasma_reference/plasma_nlte_reference.h5'


@vg3095 can you check out how this refdata is solved in carsus and use the same methods

yeganer · 2017-08-07T08:54:32Z

@vg3095 as @wkerzendorf pointed out I have some ideas how we could solve the issue with the reference data for tardis. The idea borrows the approach from carsus in some way.
I would propose to move ALL reference data to the tardis-refdata repository and then rename the config option atomic-dataset to tardis-refdata which points to a checkout of said repository.

On the tardis side this has the benefit that we can treat all file names as relative to this 'root'. What do you think? Would this be a good idea?

vg3095 · 2017-08-07T11:02:41Z

@yeganer I think its a great idea.

yeganer · 2017-08-07T12:27:08Z

@vg3095 With this approach, we should also test for the version of the tardis-refdata repository and fix this version in tardis. So specific tests would need version > 0.1 for example.

vg3095 · 2017-08-08T07:49:20Z

@wkerzendorf You can review this PR now.

yeganer

@vg3095 I did a quick review of the PR and found some only small issues. However, it was very difficult for me to review because this PR mixes two changes: The transition towards the tardis-refdata repository and your changes to the plasma tests. Could you please split these into two Pull Requests? This would make reviewing a lot easier.
Apart from that, I think the changes required to use tardis-refdata seem to be almost complete. Good and very quick work!

yeganer · 2017-08-08T08:25:33Z

tardis/plasma/tests/test_complete_plasmas.py


        cls.config_path = os.path.join(
            'tardis', 'plasma', 'tests', 'data', 'plasma_test_config_nlte.yml')
        cls.config = Configuration.from_yaml(cls.config_path)
-        cls.config['atom_data'] = os.path.abspath(os.path.expanduser(os.path.expandvars(
-            pytest.config.getvalue('atomic-dataset'))))
+        cls.config['atom_data'] = os.path.join(


You should use the one of the atomic_dataset fixtures here.

yeganer · 2017-08-08T08:26:52Z

tardis/tests/test_atomic.py

-@pytest.mark.skipif(not pytest.config.getvalue("atomic-dataset"),
-                    reason='--atomic_database was not specified')
+@pytest.mark.skipif(not pytest.config.getvalue("tardis-refdata"),
+                    reason='--tardis-refdata was not specified')
 def test_atomic_reprepare():


Again, we should use a fixture here.

yeganer · 2017-08-08T08:27:15Z

tardis/tests/test_tardis_full.py

@@ -25,7 +25,7 @@ class TestSimpleRun():
    @pytest.fixture(scope="class", autouse=True)
    def setup(self):
        self.atom_data_filename = os.path.expanduser(os.path.expandvars(
-            pytest.config.getvalue('atomic-dataset')))
+            os.path.join(pytest.config.getvalue('tardis-refdata'), 'atom_data', 'kurucz_cd23_chianti_H_He.h5')))


As above, this should be a fixture.

yeganer · 2017-08-08T08:31:51Z

tardis/plasma/tests/test_complete_plasmas.py

+from tardis.simulation import Simulation
+
+
+class BasePlasmaTest:


This is missing the (object).

wkerzendorf · 2017-08-09T08:46:20Z

@vg3095 is this rebased on #775 ?

vg3095 · 2017-08-09T09:04:23Z

@wkerzendorf Not completely. This misses 2 commits, which I added afterwards in PR #775. I have left it deliberately for now, so that travis.yml uses PR#2 branch reference files. If you could merge Plasma reference files in PR#2, then I could update this.

vg3095 · 2017-08-10T09:17:45Z

@wkerzendorf You can review this PR. I have rebased it.
I have added some new tests , which I have mentioned in the description of the PR.

wkerzendorf · 2017-08-11T08:31:56Z

tardis/plasma/tests/test_complete_plasmas.py

+
+class TestNLTEPlasma(BasePlasmaTest):
+
+    @classmethod


why is this a classmethod?

wkerzendorf · 2017-08-11T08:32:27Z

tardis/plasma/tests/test_complete_plasmas.py

+    @classmethod
+    @pytest.fixture(scope="class", autouse=True)
+    def setup(cls, atomic_data_fname, tardis_ref_path):
+        cls.reference_file_path = os.path.join(


same code as above - why is this not in the baseclass

wkerzendorf · 2017-08-11T08:34:00Z

tardis/plasma/tests/test_complete_plasmas.py

+    #GENERAL PROPERTIES
+    def test_beta_rad(self):
+        pdt.assert_almost_equal(self.plasma.beta_rad,
+                                self.read_hdf_attr('beta_rad').values)


this could be parameterizable - that might make it easier.

yeganer · 2017-08-10T12:34:31Z

.travis.yml

@@ -70,6 +70,8 @@ before_install:
    - if [[ $TEST_MODE == 'spectrum' ]]; then git lfs install --skip-smudge; fi
    - if [[ $TEST_MODE == 'spectrum' ]]; then git clone $TARDIS_REF_DATA_URL $HOME/tardis-refdata; fi
    - if [[ $TEST_MODE == 'spectrum' ]]; then cd $HOME/tardis-refdata; fi
+    - if [[ $TEST_MODE == 'spectrum' ]]; then git fetch origin pull/2/head:plasma-ref2; fi


This is only temporary, right?

Yes, this is temporary.

yeganer · 2017-08-10T12:38:15Z

tardis/plasma/tests/test_complete_plasmas.py

+    #GENERAL PROPERTIES
+    def test_beta_rad(self):
+        pdt.assert_almost_equal(self.plasma.beta_rad,
+                                self.read_hdf_attr('beta_rad').values)


Why are we using x.values here instead of the original object? Is the reference not a DataFrame/Series or is there another reason for this?

yeganer · 2017-08-10T12:40:50Z

tardis/plasma/tests/test_complete_plasmas.py

+        cls.config = Configuration.from_yaml(cls.config_path)
+        cls.config['atom_data'] = atomic_data_fname
+        cls.sim = Simulation.from_config(cls.config)
+        cls.sim.run()


I think it's not required to run the Simulation. Everything that depends on the run is either tested in the integration tests or in test_tardis_full.

vg3095 · 2017-08-11T13:56:02Z

@wkerzendorf I have updated this PR.

yeganer · 2017-08-15T19:24:47Z

I don't have specific comments on the PR however, the coverage dropped by 6%. This is not acceptable, even more so for a PR that targets tests and has full coverage of the plasma module as the ultimate goal. Please have a look at the coverage report and fix this.

vg3095 · 2017-08-16T05:19:36Z

@yeganer @wkerzendorf I have made a PR #779 regarding this, can you check that?
Currently coverage report is saved, when tests run without tardis-refdata parameter. Tests for Plasma, depend on this parameter(tardis-refdata), and that's why coverage report does not correctly reflect the status for same.

yeganer · 2017-08-16T07:39:29Z

@vg3095 I agree with that. We had a discussion about that topic sometime last year. If I remember correctly we decided to not test during the run with reference files because we were interested in the coverage of our unit-tests, not the integration tests.

With the addition of the integration tests and our plan to actively use tardis-refdata for the reference values of the unit tests, I think it is a sensible change to do the coverage run with reference data.
@wkerzendorf What are your thoughts?

wkerzendorf · 2017-08-16T08:23:19Z

@vg3095 please rebase this and then see how high the coverage is for plasma.

vg3095 · 2017-08-16T08:51:49Z

@wkerzendorf I have rebased this.

There is a 1% fall due to changing of atomic-dataset. Previously Plasma fixtures were calculated using chianti_he_db dataset, now the Plasma config files uses kurucz_cd23_chianti_H_He dataset. So, as far as I can understand from the coverage report, lines related to collision_data in tardis/atomic.py does not get covered using kurucz dataset.
Coverage of Plasma related files is constant because, test_hdf_plasma.py also compares plasma attributes from HDF file.

test_complete_plasmas.py contains combined tests for all these files

1. Parametrize Plasma tests for different properties 2. Use a common setup method , which is implemented in BasePlasmaTest class

Added 2 more Plasma setup for testing. One having radiative_rates_type - detailed, classical_nebular- True. Second having radiative_rates_type as blackbody.

yeganer · 2017-08-18T17:03:32Z

tardis/plasma/tests/test_complete_plasmas.py

+from tardis.simulation import Simulation
+
+
+setupI = {


I was more thinking about having a list of all options like this:

ionization = [ { 'ionization': 'nebular'}, { 'ionization': 'lte'}, ] excitation = [ { 'excitation' : 'lte'}, { 'excitation': 'dilute-lte'} ] @pytest.fixture(params=ionization+excitation) def config(...): for k, v in ...params.items(): config.plasma[k] = v # rest of the code

That way we test each option individually which makes it easier to see which option causes a failure.

I'm not sure right now but I think we might do this seperately for nlte and lte configurations but that should not be your concern. Your main goal should be to design this system in such a way that we can easily add the rest.

@yeganer Oh, then I cannot think of any way of setting reference_path. And, apart from that , saving reference file for all the generated permutations ( even assuming we have 4 options to set(i.e. ionization, excitation, etc), and having 2 different values that can be set for each option - 2^4 = 16 reference files(the real no. will be much greater than that, considering all options).

@yeganer Did you mean , that like for example , ionization : nlte , that the values will be same, irrespective of other plasma config options?

We don't need to test all permutations. That would be too much reference data. But we can test all options 'alone'. I think this would give us ~20 different setups which I think is doable.

yeganer · 2017-08-18T17:39:11Z

For the reference path, one method could be to hash the string representation of the config. The result should be unique and one could use this as the identification of the reference data set.
Additionally I think, we should store ALL data in the same HDF store and use the hash as the key in the HDFStore (instead of saving to /plasma/)

wkerzendorf · 2017-08-21T12:31:44Z

@vg3095 that looks great - can you tell us what the test percentage is for plasma - and what and if things are missing.

wkerzendorf · 2017-08-22T08:08:36Z

tardis/io/util.py

@@ -282,6 +282,8 @@ def get_properties(self):
        data['atom_data_uuid'] = self.atomic_data.uuid1
        if 'atomic_data' in data:
            data.pop('atomic_data')
+        if 'nlte_data' in data:


can you add a warning here that nlte_data can not be saved.

wkerzendorf · 2017-08-22T08:27:29Z

@vg3095 I wrote some comments on gitter.

vg3095 force-pushed the plasma_tests branch from 56ba1d0 to ccc24be Compare August 7, 2017 05:18

vg3095 commented Aug 7, 2017

View reviewed changes

wkerzendorf reviewed Aug 7, 2017

View reviewed changes

yeganer suggested changes Aug 8, 2017

View reviewed changes

vg3095 force-pushed the plasma_tests branch 2 times, most recently from d1ce48c to 0d76524 Compare August 9, 2017 03:50

vg3095 force-pushed the plasma_tests branch from 0d76524 to ddd6ba8 Compare August 10, 2017 08:43

vg3095 changed the title ~~[WIP] Replace Comparison values in Plasma Unit Tests with reference HDF file~~ Replace Comparison values in Plasma Unit Tests with reference HDF file Aug 10, 2017

wkerzendorf reviewed Aug 11, 2017

View reviewed changes

yeganer reviewed Aug 11, 2017

View reviewed changes

vg3095 force-pushed the plasma_tests branch 2 times, most recently from 239f749 to d19f0da Compare August 11, 2017 12:50

vg3095 closed this Aug 11, 2017

vg3095 reopened this Aug 11, 2017

vg3095 force-pushed the plasma_tests branch from 3ec4f98 to ec429e2 Compare August 16, 2017 08:24

vg3095 force-pushed the plasma_tests branch from 2e0a628 to f723431 Compare August 18, 2017 16:31

vg3095 added 16 commits August 18, 2017 22:10

Delete old plasma test files

fccbf1a

test_complete_plasmas.py contains combined tests for all these files

Add generate-reference option for Plasma Tests

4060d4e

Fix indentation inside TestNLTEPlasma

38ebff8

Use atomic_data_fname fixture in plasma tests

9cd60b3

Use git checkpoint of PR#2 for tardis-refdata

6b548dc

Use tardis_ref_path fixture in plasma tests

34e0412

Parametrize plasma tests

79ff2fc

1. Parametrize Plasma tests for different properties 2. Use a common setup method , which is implemented in BasePlasmaTest class

Combine list of properties in plasma tests

a5e3e1e

Use plasma as a class level fixture in tests

4e49f52

Use yield fixture for HDF file in plasma tests

a5f3758

Add some more plasma tests

74d2eb6

Added 2 more Plasma setup for testing. One having radiative_rates_type - detailed, classical_nebular- True. Second having radiative_rates_type as blackbody.

Fix to use chianti_he_db dataset for NLTE config

32f1efa

Update plasma tests to use chianti_he_db dataset

ad965c8

Generate different plasma config using fixture

f914c81

Add a base plasma config

cf59431

Remove unused fixture

bf4c5d4

vg3095 force-pushed the plasma_tests branch from f723431 to bf4c5d4 Compare August 18, 2017 16:40

yeganer reviewed Aug 18, 2017

View reviewed changes

vg3095 added 5 commits August 19, 2017 08:21

Generate plasma tests using list of config options

cde7889

Fix negative power error in HeliumNLTE

cd1a6df

Test for recomb-nlte option in plasma

8e52780

Add plasma test for delta treatment

3632663

Use w_epsilon when radiative_rates_type is detailed in plasma tests

b2b53ce

wkerzendorf reviewed Aug 22, 2017

View reviewed changes

Add warning that nlte_data can't be saved

d178387

wkerzendorf merged commit f43076f into tardis-sn:master Aug 22, 2017

		from tardis.simulation import Simulation


		class BasePlasmaTest:

Replace Comparison values in Plasma Unit Tests with reference HDF file #774

Replace Comparison values in Plasma Unit Tests with reference HDF file #774

Conversation

vg3095 commented Aug 3, 2017 • edited Loading

vg3095 commented Aug 3, 2017 • edited Loading

vg3095 commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeganer commented Aug 7, 2017

vg3095 commented Aug 7, 2017

yeganer commented Aug 7, 2017

vg3095 commented Aug 8, 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

wkerzendorf commented Aug 9, 2017

vg3095 commented Aug 9, 2017

vg3095 commented Aug 10, 2017

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

vg3095 commented Aug 11, 2017

yeganer commented Aug 15, 2017

vg3095 commented Aug 16, 2017

yeganer commented Aug 16, 2017

wkerzendorf commented Aug 16, 2017

vg3095 commented Aug 16, 2017

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 Aug 18, 2017

wkerzendorf commented Aug 21, 2017

Choose a reason for hiding this comment

wkerzendorf commented Aug 22, 2017

vg3095 commented Aug 3, 2017 •

edited

Loading

vg3095 commented Aug 3, 2017 •

edited

Loading