Bugfix radiative rates type detailed

[chvogl]

In the current version of TARDIS the detailed mode for the estimation of radiative rates does not work (see #487). This PR fixes the problem.

[wkerzendorf]

I guess these things are only slow-testable. So merge now and do a test later - what do people think @tardis-sn/tardis-core

[unoebauer]

If we want to properly test it, we need a slow test. However, one could think about simple, quick test, which just runs Tardis with few packets in the detailed mode and checks that it doesn't break and that j_blues are returned.

@chvogl, @wkerzendorf - thoughts on this?

[unoebauer]

@wkerzendorf - with the last commit, we have introduced a simple test to check the j_blue_estimators. Naturally, the slow test should probe the detailed mode thoroughly.

Still, a simple test is in place and I'd merge it - objections, @wkerzendorf, @orbitfold?

[wkerzendorf]

@unoebauer - signing off on this!

[orbitfold]

I don't like the way this is indented, the bottom line in the multiplication should be indented one level to the right.

[wkerzendorf]

Is this a specific PEP8 violation?

[orbitfold]

Don't know but it should be. Now it looks like it is another argument at first glance and not a part of an expression.

[wkerzendorf]

You are the style person - can you suggest how to make it PEP8 conform?

[yeganer]

There are people that fancy the operator on the next line for that reason.
(Which is against PEP)

The pythonic way might be to save the calculation in a temporary variable
and pass that as the argument.

Wolfgang Kerzendorf notifications@github.com schrieb am Di., 16. Feb.
2016 17:11:

In tardis/model.py
#486 (comment):

@@ -207,7 +207,7 @@ def calculate_j_blues(self, init_detailed_j_blues=False):
logger.info('Calculating J_blues for radiate_rates_type=detailed')

         self.j_blues = pd.DataFrame(

•            self.j_blue_estimators.transpose() *
  

•            self.j_blue_estimators *
           self.j_blues_norm_factor.value,
  

You are the style person - can you suggest how to make it PEP8 conform?

—
Reply to this email directly or view it on GitHub
https://github.com/tardis-sn/tardis/pull/486/files#r53032635.

[orbitfold]

PEP8 does not address everything. And in a lot of cases it is not entirely clear what is meant. For example it says to use blank lines inside method bodies "sparingly, to indicate logical sections". What does that mean? I think 80% of TARDIS's Python code is in violation of that.

[unoebauer]

Ok - it seems as if this innocent little line of code is sparking a fundamental discussion of TARDIS coding style. I'd suggest we address this properly either in a TEP or on the tardis documentation (after first agreeing on a style, naturally) and just get on with the fix at hand.

Pragmatic solution - just leave it as it is until we have decided on conventions for cases which are not unambiguously covered by PEP8 and merge the PR

[yeganer]

After getting informed more about coding styles I have to admit indenting the next line would look better and improve readability.

We should definitely write some code guidelines for the documentation and every time we encounter something not handled yet add it.

[wkerzendorf]

@yeganer can you do a PR on @chvogl's PR and implement that then.

[unoebauer]

Guys - let's not get sidetracked here - we want to fix the detailed mode! Let's merge and not get bugged down in style questions (which we also have not yet been formalised!)

[unoebauer]

Happy with it @chvogl?

[wkerzendorf]

Okay @yeganer @orbitfold - we will merge this PR tomorrow at 2pm. If you want to change the current style, make a PR to this PR by then.