Quickfix to solve Issue #455 and #464 #471

yeganer · 2016-02-01T13:33:58Z

Deleted atom_data._lines and atom_data._levels
This change ensures that the BasePlasma always gets prepared data.
A proper fix will take more time and will be done later.

Deleted atom_data._lines and atom_data._levels This change ensures that the BasePlasma always gets prepared data. A proper fix will take more time and will be done later.

unoebauer · 2016-02-01T13:36:51Z

I strongly suspect that the Travis testing process will fail, at lest for the full_spectrum test. The spectra won't be bit-equal to the recorded values. You should re-calculate these tests offline, ensure that the new results are sensible and update the data files.

wkerzendorf · 2016-02-01T14:10:18Z

@yeganer looks very good, we need to think about updating the tests. maybe grab a coffee now at ESO?

Because they didn't call it and it's mandatory All default tests are working now. Spectral test might need updating.

There are still 3 tests failing: test_packet_output test_plasma_estimates TestPlasma.test_lte_plasma test_atomic_reprepare needs a small rewrite/fix (see comments in code)

The old files contained wrong values (see issue tardis-sn#455) With this change the following tests will run again: * test_packet_output * test_plasma_estimates * TestPlasma.test_lte_plasma One should consider updating tardis/simulation/tests/test_simulation.py since it creates a new model which essentially tests the whole codebase. These tests will always fail if something in the codebase changes ( see comment in the file ).

yeganer · 2016-02-02T13:40:26Z

@wkerzendorf @unoebauer: I fixed all tests. They are running fine locally. If you could please look over the changes and run the test locally so we can be sure everything is working fine and we can merge this and finally fix the bug for master.

There are still 2 Issues with the code.

test_atomic.py:test_atomic_reprepare() should be rewritten and the logic should be changed( see comment in code) to check for len(lines) == N and not if len(lines) > 0.
test_simulation.py seems like a "small" version of a full tardis test to me. Either move the logic ( comparing nubar, t_rad, w, j_est) to test_tardis_full.py or load a fixed model with a known plasma state and run the simulation on that. The current implementation will fail on any change in the codebase, whether it is in plasma, atomic, montecarlo or somewhere else. In that case the bug would be catched by test_tardis_full.py anyway.

Since these have nothing to do with the bug, I'll open new Issues for them. I just wanted to document them.

unoebauer · 2016-02-02T14:00:54Z

tardis/plasma/properties/atomic.py

@@ -58,18 +59,19 @@ class Lines(BaseAtomicDataProperty):
    outputs = ('lines', 'nu', 'f_lu', 'wavelength_cm')

    def _filter_atomic_property(self, lines, selected_atoms):
-        return lines[lines.atomic_number.isin(selected_atoms)]
+        # return lines[lines.atomic_number.isin(selected_atoms)]
+        return lines

    def _set_index(self, lines):
 # Filtering process re-arranges the index. This re-orders it.
 # It seems to be important that the lines stay indexed in the correct order
 # so that the tau_sobolevs values are in the right order for the montecarlo
 # code, or it returns the wrong answer.


I would really remove the comments - this may lead to confusions in the future if somebody looks at it and doesn't remember the entire #455 discussion

My reasoning behind not removing it was that someday in the future when all the filtering and atom data preparation gets implemented in the plasma part having the old functionality at hand might be useful.

But your comment reminded me that BasicAtomDataProperty definitely needs a change.

Hmm - but I think that the entire idea behind the enforced re-indexing (which should only achieve compatibility between unsorted _lines and sorted lines) is no longer applicable. Moreover, the re-indexing procedure left in the comments is wrong. Given all that, I'd strongly suggest removing the comments.

Remove numpy and pytest as imports from test_plasma_simple.py as they were not needed.

Fix BaseAtomicDataProperty:calculate(): It won't try to load atom_data._<property> anymore since. This is a cleanup since atom_data._<property> doesn't exist anymore.

unoebauer · 2016-02-02T15:54:27Z

Apart from my concern about the comments in the Plasma part, I am happy with this PR and am in favour of merging ASAP. Opinions, @aoifeboyle, @wkerzendorf, @orbitfold?

yeganer · 2016-02-02T16:32:09Z

tardis/plasma/properties/atomic.py

+        #     reindexed = lines.reindex(lines.index)
+        # except:
+        #     reindexed = lines.reindex(lines.index)
+        # lines = reindexed.dropna(subset=['atomic_number'])


@unoebauer I'm fine with removing that part since you are right, it is wrong and obsolete. However i'd leave the line filtering in place and maybe add a commented part to _set_index() which mirrors the current set_index that is done in atomic.py

Fine with me.

I think that is a good idea - leave the _set_index method in there even if it doesn't do anything.

yeganer · 2016-02-02T21:12:00Z

@unoebauer Could you please run the original setup where this bug first appeared and compare the output with 45eca24, just to be 100% sure.

wkerzendorf · 2016-02-03T10:57:26Z

tardis/conftest.py

@@ -133,10 +133,10 @@ def pytest_report_header(config):
    if opts:
        s += "Using Astropy options: {0}.\n".format(" ".join(opts))

-    if six.PY3 and (config.getini('doctest_rst') or config.option.doctest_rst):
+    if not six.PY2 and (config.getini('doctest_rst') or config.option.doctest_rst):


what's happening here?

It's changing the logic to treat python2 as the exception. That way this part doesn't need to be rewritten in the future once there is python 3 and python 4 around.
The old code would treat python4 the same way as python2 which would result in an undesired behavior.

I wouldn't worry about Python 4. Even if it does eventually happen, I doubt compatibility issues will be handled by the six module, which is purely for compatibility issues between 2 and 3.

I just ran across that and thought, why not fix it. If I run across a potential bug, is there any reason to not fix it?

I'd argue the original behavior is not a bug. Either way it's a minor issue. Either way is probably fine. Just I don't think the change was really needed.

okay - I think we should leave it as the old one. The only reason is that when our astropy friends will help us, they are not confused. please change it back.

wkerzendorf · 2016-02-03T11:03:23Z

@yeganer - I've added a few comments, but it is almost done.

Changed behavior to raise a custom exception instead of a generic one. This is done to better reflect the error. Related Issues: tardis-sn#455 tardis-sn#464 Pull Request tardis-sn#471

The comments were describing the problem with the tests in this file. An issue has been created regarding this problem rendering the comments obsolete. Related Issue: tardis-sn#474

The old, commented code was wrong and got replaced with the code for a future implementation. Pull Request tardis-sn#471

yeganer · 2016-02-03T13:27:07Z

@wkerzendorf I addressed most of your concerns. The only remaining issues are:

verysimpleconfig.yaml restore original behavior?
PY2/PY3 restore original behavior?

If that's settled, we can merge.

wkerzendorf · 2016-02-03T13:57:01Z

tardis/io/tests/data/tardis_configv1_verysimple.yml

@@ -5,7 +5,6 @@ supernova:
    time_explosion: 13 day

 atom_data: kurucz_atom_pure_simple.h5


this is fine - if as you say this is never really used we could put a dummy string in. but leave it for now.

wkerzendorf · 2016-02-03T13:58:43Z

I think checkout conftest.py and then merge from my side.
@orbitfold - do you agree to merge?
@unoebauer - do you agree to merge?

orbitfold · 2016-02-03T14:04:24Z

Fine by me.

unoebauer · 2016-02-03T17:38:25Z

@wkerzendorf Just a second - let me do the last final check. I'd like to rerun the original #455 problem. If this passes let's merge.

unoebauer · 2016-02-03T18:24:55Z

@wkerzendorf The test looks excellent:

Let's merge

Quickfix to solve Issue #455 and #464

Quickfix to solve Issue tardis-sn#455 and tardis-sn#464

43239cc

Deleted atom_data._lines and atom_data._levels This change ensures that the BasePlasma always gets prepared data. A proper fix will take more time and will be done later.

yeganer added 3 commits February 1, 2016 15:43

Added AtomData.prepare_atom_data() to plasma tests

feb413e

Because they didn't call it and it's mandatory All default tests are working now. Spectral test might need updating.

Changed reference files for test_tardis_full

8ad2d87

There are still 3 tests failing: test_packet_output test_plasma_estimates TestPlasma.test_lte_plasma test_atomic_reprepare needs a small rewrite/fix (see comments in code)

unoebauer reviewed Feb 2, 2016
View reviewed changes

yeganer mentioned this pull request Feb 2, 2016

Improve tests of simulation object #474

Closed

yeganer added 2 commits February 2, 2016 15:32

Remove obsolete import, PEP8 stuff

6f7d2f5

Remove numpy and pytest as imports from test_plasma_simple.py as they were not needed.

Remove legacy code from plasma/properties/base.py

ebeb435

Fix BaseAtomicDataProperty:calculate(): It won't try to load atom_data._<property> anymore since. This is a cleanup since atom_data._<property> doesn't exist anymore.

yeganer reviewed Feb 2, 2016
View reviewed changes

wkerzendorf reviewed Feb 3, 2016
View reviewed changes

yeganer added 3 commits February 3, 2016 13:45

Fix exception raising in prepare_atom_data

6d31198

Changed behavior to raise a custom exception instead of a generic one. This is done to better reflect the error. Related Issues: tardis-sn#455 tardis-sn#464 Pull Request tardis-sn#471

Remove comments in test_simulation.py

b2578e3

The comments were describing the problem with the tests in this file. An issue has been created regarding this problem rendering the comments obsolete. Related Issue: tardis-sn#474

Updated comments in Lines._set_index

0ad795a

The old, commented code was wrong and got replaced with the code for a future implementation. Pull Request tardis-sn#471

Revert tardis_configv1_verysimple.yml to original

5c9bf70

wkerzendorf reviewed Feb 3, 2016
View reviewed changes

Revert python4 compatibility in tardis/confest.py

6ee5fca

unoebauer added a commit that referenced this pull request Feb 3, 2016

Merge pull request #471 from yeganer/quickfix_455

9b9feda

Quickfix to solve Issue #455 and #464

unoebauer merged commit 9b9feda into tardis-sn:master Feb 3, 2016

yeganer mentioned this pull request Feb 7, 2016

Probably fix for #455. ( Based on current master) #466

Closed

mishinma mentioned this pull request Feb 7, 2016

test_simulation.py fails with the latest version of numpy #478

Closed

unoebauer mentioned this pull request Feb 8, 2016

Wrong nubar_estimators (potentially also inconsistencies in transition probabilities) -- follow-up of #455 #464

Closed

yeganer deleted the quickfix_455 branch May 18, 2016 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quickfix to solve Issue #455 and #464 #471

Quickfix to solve Issue #455 and #464 #471

yeganer commented Feb 1, 2016

unoebauer commented Feb 1, 2016

wkerzendorf commented Feb 1, 2016

yeganer commented Feb 2, 2016

unoebauer Feb 2, 2016

yeganer Feb 2, 2016

unoebauer Feb 2, 2016

unoebauer commented Feb 2, 2016

yeganer Feb 2, 2016

unoebauer Feb 2, 2016

wkerzendorf Feb 3, 2016

yeganer commented Feb 2, 2016

wkerzendorf Feb 3, 2016

yeganer Feb 3, 2016

orbitfold Feb 3, 2016

yeganer Feb 3, 2016

orbitfold Feb 3, 2016

wkerzendorf Feb 3, 2016

wkerzendorf commented Feb 3, 2016

yeganer commented Feb 3, 2016

wkerzendorf Feb 3, 2016

wkerzendorf commented Feb 3, 2016

orbitfold commented Feb 3, 2016

unoebauer commented Feb 3, 2016

unoebauer commented Feb 3, 2016

		@@ -5,7 +5,6 @@ supernova:
		time_explosion: 13 day

		atom_data: kurucz_atom_pure_simple.h5

Quickfix to solve Issue #455 and #464 #471

Quickfix to solve Issue #455 and #464 #471

Conversation

yeganer commented Feb 1, 2016

unoebauer commented Feb 1, 2016

wkerzendorf commented Feb 1, 2016

yeganer commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unoebauer commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeganer commented Feb 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkerzendorf commented Feb 3, 2016

yeganer commented Feb 3, 2016

Choose a reason for hiding this comment

wkerzendorf commented Feb 3, 2016

orbitfold commented Feb 3, 2016

unoebauer commented Feb 3, 2016

unoebauer commented Feb 3, 2016