[TEP006] Aggregate TARDIS restructure and configuration system cleanup #652

ftsamis · 2016-08-18T16:51:35Z

This is an aggregate of:

Restructure Radial1DModel - Restructure Radial1DModel #632
Replace LegacyPlasmaArray with a function - Replace LegacyPlasmaArray with a function #635
Restructure the MontecarloRunner - Restructure the MontecarloRunner #636
Restructure the Simulation - Restructure the Simulation #645
Cleanup of the config_reader module - Cleanup of the config_reader module. #623
Fix hold iterations - Fix hold iterations #638

wkerzendorf · 2016-11-01T16:55:00Z

tardis/plasma/base.py

@@ -14,11 +14,11 @@
 class BasePlasma(object):

    outputs_dict = {}
-    def __init__(self, plasma_properties, **kwargs):
+    def __init__(self, plasma_properties, property_args=None, **kwargs):


@ftsamis that looks awesome! I'm wondering if we should call this property_kwargs? But if you think args is better let's go with this.

You are right, property_kwargs is a better name. Done

wkerzendorf · 2016-12-05T23:55:52Z

@tardis-sn/tardis-core I would like to have as many collaboration members as possible look over this. This is a major change, but seemingly has passed all the tests for now.

… inside the Radial1DModel.

…ary slicing.

wkerzendorf · 2016-12-06T00:01:13Z

tardis/model/base.py

+logger = logging.getLogger(__name__)
+
+
+class Radial1DModel(object):


missing documentation. This needs to be fixed here or as an issue
it's not clear what the different attributes are and how the class works.

wkerzendorf · 2016-12-06T00:02:09Z

tardis/model/density.py

+from tardis.util import quantity_linspace
+
+
+class HomologousDensity(object):


same here - documentation missing and explanation.

wkerzendorf · 2016-12-06T00:02:29Z

tardis/model/density.py

+        self.density_0 = density_0
+        self.time_0 = time_0
+
+    def after_time(self, time_explosion):


not clear if this function name is clear - @tardis-sn/tardis-core ?

I agree with @wkerzendorf

Propose an alternative.

calculate_density_evolution? @chvogl If no other alternative in the next hour just use that and we are done!!

I think something like calculate_density_at_time_of_simulation would be more comprehensible.

I really don't like long names, but done.

wkerzendorf · 2016-12-06T00:03:52Z

tardis/model/density.py

+                                            time_explosion).cgs
+
+    @classmethod
+    def from_config(cls, config):


documentation missing

Fixed. Also for all other from_config classmethods.

wkerzendorf · 2016-12-06T00:05:17Z

tardis/montecarlo/base.py

@@ -110,29 +120,30 @@ def legacy_update_spectrum(self, no_of_virtual_packets):
            self.spectrum_virtual.update_luminosity(
                self.montecarlo_virtual_luminosity)

-    def run(self, model, no_of_packets, no_of_virtual_packets=0, nthreads=1,last_run=False):
+    def run(self, model, plasma, no_of_packets, no_of_virtual_packets=0, nthreads=1,last_run=False):


We should fix the documentation here.

wkerzendorf · 2016-12-06T00:06:11Z

tardis/montecarlo/montecarlo.pyx

    exptau = 1 - np.exp(-
                        runner.line_lists_tau_sobolevs.reshape(-1,
                            runner.j_estimator.shape[0]) )
-    model.Edotlu = Edotlu_norm_factor*exptau*runner.Edotlu_estimator
+    runner.Edotlu = Edotlu_norm_factor*exptau*runner.Edotlu_estimator


PEP 8 violation

Out of scope for this PR. Introduced in #595.

chvogl · 2016-12-16T09:46:30Z

tardis/io/model_reader.py

-
-    mean_densities = mean_densities[inner_boundary_index:outer_boundary_index]
-
+    time_of_model,velocity, unscaled_mean_densities = file_parsers[filetype](filename)


PEP8 Violation

chvogl · 2016-12-16T10:03:04Z

tardis/model/density.py

+        self.density_0 = density_0
+        self.time_0 = time_0
+
+    def after_time(self, time_explosion):


I agree with @wkerzendorf

chvogl · 2016-12-16T10:11:20Z

tardis/model/density.py

+        if d_conf.type == 'branch85_w7':
+            density_0 = calculate_power_law_density(v_middle, d_conf.w7_v_0,
+                                                    d_conf.w7_rho_0, -7)
+            return cls(density_0, d_conf.w7_time_0)


I think it would be nicer to have the returns outside the if statements. Just set time_0 equal to d_conf.w7_time_0 for this case and move the return to the end.

I agree. Fixed.

chvogl · 2016-12-16T10:11:58Z

tardis/model/density.py

+def calculate_power_law_density(velocities, velocity_0, rho_0, exponent):
+    """
+
+    This function computes a descret exponential density profile.


Out of scope for this PR, introduced in #26.

chvogl · 2016-12-16T10:20:32Z

tardis/montecarlo/base.py

        self.packet_source = packet_source.BlackBodySimpleSource(seed)
        self.spectrum_frequency = spectrum_frequency
+        self.virtual_spectrum_range = virtual_spectrum_range # TODO: linspace handling


If this is still relevant it would be nice to make an issue out of this. Otherwise just remove the TODO.

This is closely related to #644. I removed the TODO comment for now. Fixed.

chvogl · 2016-12-16T10:31:16Z

tardis/montecarlo/base.py

+    def from_config(cls, config):
+        if config.plasma.disable_electron_scattering:
+            logger.warn('Disabling electron scattering - this is not physical')
+            sigma_thomson = 1e-200 / (u.cm ** 2)


Cross sections have units of (length)**2.

The logic was actually copied as-is from current master:

tardis/tardis/io/config_reader.py

Line 948 in 60d1d1d

validated_config_dict['montecarlo']['sigma_thomson'] = 6.652486e-25 / (u.cm ** 2)

.

Since I don't really know what sigma_thomson or cross sections are in this context, I'd like a confirmation on if we should change that. @wkerzendorf?

After some digging in the code, it seems that the unit of sigma_thomson is completely ignored afterwards, since cmontecarlo uses just its value. So, neither the wrong unit, neither correcting it would affect the computations in any way, which means it is probably safe to fix this by directly using, as @chvogl proposed, the astropy.constants.sigma_T.

Fixed as proposed.

chvogl · 2016-12-16T10:34:11Z

tardis/montecarlo/base.py

+            sigma_thomson = 1e-200 / (u.cm ** 2)
+        else:
+            logger.debug("Electron scattering switched on")
+            sigma_thomson = 6.652486e-25 / (u.cm ** 2)


Same as above.

Maybe we should use the Thomson scattering cross-section from astropy (astropy.constants.sigma_T) instead of hardcoding the value.

Same answer as above. @wkerzendorf. On your second comment, if there is a constant for that it seems perfectly reasonable to use it as far as I am concerned.

Fixed as proposed.

chvogl · 2016-12-16T10:37:48Z

tardis/montecarlo/base.py

+
+        return cls(seed=config.montecarlo.seed,
+                   spectrum_frequency=spectrum_frequency,
+                   # TODO: Linspace handling for virtual_spectrum_range


Maybe make an issue out of this.

Removed the TODO comment. Fixed.

…y_at_time_of_simulation

…d of in every 'if'.

…ing its unit.

unoebauer

Had a very quick look at it - looks very impressive to me, good job @ftsamis! Apart from the legacy support thing (the violation of which I am ok with), nothing stuck out. I am all for merging and closing this chapter.

unoebauer · 2016-12-20T08:55:02Z

tardis/base.py


-    return radial1d_mdl
+    return simulation


This will break existing scripts which expect that radial1d_mdl is returned by run_tardis.

I am tempted to swallow this and will update e.g. tardisanalysis.

This is true, although having the entire simulation object returned is much more useful and takes the need to duplicate information, which we heavily did until now, i.e. copying various stuff to model.

So, swallowing this would be a fair trade-off, I believe. ☺

wkerzendorf · 2016-12-20T10:33:05Z

@ftsamis undo the last change. this breaks the tests - we shouldn't do this in this PR.

…also fixing its unit." This reverts commit 136f667.

ftsamis · 2016-12-20T13:23:30Z

~~@wkerzendorf That's because I forgot to convert sigma_T to cgs.~~ I agree though that this could be best done in a different PR, so I reverted the change.

Edit: I had converted sigma_T to cgs. No idea what caused the tests to fail.

wkerzendorf · 2016-12-20T13:47:41Z

@ftsamis congratulations - I'm merging this now.

ftsamis added needs review configuration labels Aug 18, 2016

wkerzendorf added the in progress label Aug 18, 2016

ftsamis force-pushed the restructure-aggregate branch 2 times, most recently from 1585d7f to a12700b Compare August 19, 2016 22:28

This was referenced Sep 12, 2016

Fix hold iterations #638

Closed

Restructure the Simulation #645

Closed

Restructure the MontecarloRunner #636

Closed

Replace LegacyPlasmaArray with a function #635

Closed

Restructure Radial1DModel #632

Closed

wkerzendorf reviewed Nov 1, 2016

View reviewed changes

ftsamis force-pushed the restructure-aggregate branch 5 times, most recently from 5a1b4de to 7e443d6 Compare November 12, 2016 21:11

ftsamis force-pushed the restructure-aggregate branch from 7e443d6 to 81ee84c Compare November 19, 2016 13:49

ftsamis force-pushed the restructure-aggregate branch from 81ee84c to 48ceb8c Compare December 2, 2016 02:26

ftsamis removed the in progress label Dec 6, 2016

ftsamis added 5 commits December 8, 2016 15:37

Upgrade the model module to a python package.

98f1250

Introduce the basics of the new model/base.py.

0c400a8

Add a dynamic density property to the new model.

4bbd4b7

Add a volume and a no_of_shells property to the model.

f74de3a

Add a radius property to the Radial1DModel.

9ea458a

ftsamis added 3 commits December 8, 2016 15:37

Fix model.t_radiative not being cut to the specified v boundaries.

848a64e

Fix slicing the t_radiative twice in the getter if it was initialized…

450fc17

… inside the Radial1DModel.

Fix a wrong check in Radial1DModel.t_radiative setter regarding bound…

5640c5f

…ary slicing.

ftsamis force-pushed the restructure-aggregate branch from ca2684c to 5640c5f Compare December 8, 2016 13:37

Fix the luminosity_inner hdf location in test_integration.

b85aea9

ftsamis requested review from wkerzendorf, orbitfold, yeganer and unoebauer December 8, 2016 20:06

wkerzendorf requested changes Dec 15, 2016

View reviewed changes

wkerzendorf requested review from chvogl and removed request for yeganer December 16, 2016 09:41

chvogl reviewed Dec 16, 2016

View reviewed changes

ftsamis added 6 commits December 16, 2016 19:37

Add docstrings for some classes and methods affected by the restructure.

02506b9

Fix a PEP8 violation in model_reader.

329564b

Rename the after_time method of HomologousDensity to calculate_densit…

c3b7550

…y_at_time_of_simulation

Use the return statement once in HomogolousDensity.from_config instea…

d3f5946

…d of in every 'if'.

Remove an outdated TODO comment.

782a0ab

Use sigma_T from astropy constants instead of hardcoding it, also fix…

136f667

…ing its unit.

unoebauer approved these changes Dec 20, 2016

View reviewed changes

wkerzendorf approved these changes Dec 20, 2016

View reviewed changes

Revert "Use sigma_T from astropy constants instead of hardcoding it, …

1c79c5e

…also fixing its unit." This reverts commit 136f667.

wkerzendorf merged commit b77b9e1 into tardis-sn:master Dec 20, 2016

ftsamis removed the needs review label Dec 20, 2016

ftsamis deleted the restructure-aggregate branch December 20, 2016 15:07

This was referenced Dec 20, 2016

Mark TEP006 as completed tardis-sn/tep#16

Merged

'inner_boundary_index' referenced before assignment #622

Closed

		logger = logging.getLogger(__name__)


		class Radial1DModel(object):

		from tardis.util import quantity_linspace


		class HomologousDensity(object):


		mean_densities = mean_densities[inner_boundary_index:outer_boundary_index]

		time_of_model,velocity, unscaled_mean_densities = file_parsers[filetype](filename)

[TEP006] Aggregate TARDIS restructure and configuration system cleanup #652

[TEP006] Aggregate TARDIS restructure and configuration system cleanup #652

Conversation

ftsamis commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkerzendorf commented Dec 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chvogl Dec 16, 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unoebauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkerzendorf commented Dec 20, 2016

ftsamis commented Dec 20, 2016 • edited Loading

wkerzendorf commented Dec 20, 2016

chvogl Dec 16, 2016 •

edited

Loading

ftsamis commented Dec 20, 2016 •

edited

Loading