[TEP007] Decay and merge isotopic abundance dataframe #757

vg3095 · 2017-07-04T07:01:43Z

I have added a decay method under Model class , which will take time_explosion as parameter , and then decay isotopic abundance frame , and returns merged dataframe .
See this notebook for detailed output
Jupyter Notebook

yeganer · 2017-07-04T09:12:56Z

tardis/model/base.py

@@ -189,6 +191,33 @@ def abundance(self):
        abundance.columns = range(len(abundance.columns))
        return abundance

+    def decay(self, day, normalize=True):


I think this should happen during the initialization of the model.

The behavior should be:
If there is isotope_abundance as parameter, decay these with time_explosion and merge them with the normal abundances. If we do this, save the normal abundances and the undecayed abundances somewhere on the model (example raw_abundance, raw_isotope_abundance)

wkerzendorf · 2017-07-04T11:50:06Z

tardis/model/base.py

+        isotope_abundance = self.raw_isotope_abundance.decay(time_explosion)
+
+        #Set atomic_number as index in isotopic_abundance dataframe
+        isotope_abundance = isotope_abundance.reset_index().drop(


why the many reset_index - not sure that you need those.

wkerzendorf · 2017-07-04T11:50:26Z

tardis/model/base.py

+
+        #Merge abundance dataframes
+        modified_df = isotope_abundance.append(self.abundance)
+        modified_df = modified_df.reset_index().set_index('atomic_number')


same here. you do not need to make something an index to have groupby work in it.

wkerzendorf · 2017-07-04T11:52:39Z

tardis/model/base.py

+            'mass_number', axis=1).set_index(['atomic_number'])
+
+        #Merge abundance dataframes
+        modified_df = isotope_abundance.append(self.abundance)


you definitely do not want to append - but add the decayed dataframe to the abundances.

@wkerzendorf I have now used pd.concat() in the latest commit .

If you mean pd.add() , then according to what I could find , index values for both Dataframe must be equal, then only result is correct, otherwise, values not present in one dataframe , are filled with NaN values

pd.merge() and pd.join() operation results in addition along columns.
So, I think , it leaves me with pd.concat() , and I have also added a test to ensure if it works correctly.

yeganer

This goes into the right direction, however I think we should extend the current abundances property to be more flexible so that we don't require to call a decay method.

yeganer · 2017-07-04T11:47:04Z

tardis/model/base.py

@@ -189,6 +196,33 @@ def abundance(self):
        abundance.columns = range(len(abundance.columns))
        return abundance

+    def decay(self, time_explosion, normalize=True):


I don't think we want a decay function at all. Maybe we need a helper function at some point but in general we don't need this.
We don't need functionality to 'redecay' a model because we will always use time_explosion as the time of the decay.

yeganer · 2017-07-04T11:47:52Z

tardis/model/base.py

@@ -72,7 +72,14 @@ def __init__(self, velocity, homologous_density, abundance, time_explosion,
        self.v_boundary_outer = v_boundary_outer
        self.homologous_density = homologous_density
        self._abundance = abundance
+        self.decayed = False


This is not needed.

yeganer · 2017-07-04T11:53:10Z

tardis/model/base.py

        self.time_explosion = time_explosion
+
+        self.raw_abundance = self._abundance


We should discuss and define at some point the names of all abundance variables involved.
There is input: abundance and isotope_abundance (should be attached to model for diagnosis and debugging purposes)
There is output: Model.abundance which should only contain the region defined by the velocity boundaries (as it currently does) AND which I think should contain the merged result if isotope_abundance is passed as an optional argument.
Then we have self._abundance which looks like it's currently the reference to the input (for diagnosis)

So what we basically should decide is, how do we cache the result of the decay (do we even cache it?) and how do we attach the input to the model.

@vg3095 I think the decay is extremely quick - maybe just a property would be fine. Actually can you profile how quick?

@wkerzendorf If by profile , meaning time taken by decay function here
I made a dummy dataframe, then used decay on it. (with 2 shells/columns)
It takes around 0.005 seconds for 10 elements and around 0.01 seconds for 100 elements

@wkerzendorf I have now used cProfile module, If you want to know , no. of function calls and time taken for decay method.

(Updated) No of shells/columns - 20

For 10 elements
33189 function calls (32937 primitive calls) in 0.030 seconds

For 20 elements
40899 function calls (40647 primitive calls) in 0.034 seconds

For 50 elements
64029 function calls (63777 primitive calls) in 0.050 seconds

For 100 elements
102579 function calls (102327 primitive calls) in 0.067 seconds

yeganer · 2017-07-04T12:00:39Z

I'd suggest to add a as_atomic_numbers or similar to the IsotopeAbundance class which returns the dataframe converted to the format Model.abundances currently is in.

vg3095 · 2017-07-04T13:43:58Z

As of now, these are the variables associated with abundance

raw_abundance : Normal abundance as passed during initialization
raw_isotope_abundance : Isotope abundance as passed during initialization (Undecayed)
isotope_abundance : Decayed Isotope abundance (using time_explosion)
_abundance : If raw_isotope_abundance is present , then it has combined dataframe , else
normal abundance dataframe.
abundance : Contains the region defined by the velocity boundaries. As _abundance is updated(when isotope_abundance is present), changes will reflect here also, when dataframes are merged.

vg3095 · 2017-07-04T14:06:53Z

Travis fail is due to time out in Mac build .

yeganer · 2017-07-04T16:06:35Z

I don't think we need isotope_abundance, right? Because it can be created from raw_isotope_abundance at all times

yeganer · 2017-07-04T16:07:37Z

tardis/model/base.py

@@ -189,6 +199,25 @@ def abundance(self):
        abundance.columns = range(len(abundance.columns))
        return abundance

+    def as_atomic_numbers(self, normalize=True):


This method should be part of IsotopeAbundance

wkerzendorf · 2017-07-05T12:24:48Z

tardis/io/decay.py

+
+        #Merge abundance dataframes
+        modified_df = pd.concat([isotope_abundance, abundance])


I want you to add them here

@wkerzendorf For pd.add() , index values for both Dataframe must be equal, then only result is correct, otherwise, index values not present in one dataframe , are filled with NaN values

wkerzendorf · 2017-07-05T12:29:15Z

tardis/model/base.py

@@ -63,7 +63,7 @@ class Radial1DModel(HDFWriterMixin):
    def __init__(self, velocity, homologous_density, abundance, time_explosion,
                 t_inner, luminosity_requested=None, t_radiative=None,
                 dilution_factor=None, v_boundary_inner=None,
-                 v_boundary_outer=None):
+                 v_boundary_outer=None, isotope_abundance=None):


move this up

wkerzendorf · 2017-07-05T12:29:44Z

tardis/model/base.py

@@ -73,6 +73,13 @@ def __init__(self, velocity, homologous_density, abundance, time_explosion,
        self.homologous_density = homologous_density
        self._abundance = abundance
        self.time_explosion = time_explosion
+
+        self.raw_abundance = self._abundance
+        self.raw_isotope_abundance = isotope_abundance


if it is None - you should make an empty frame

vg3095 · 2017-07-05T13:48:23Z

@wkerzendorf I have changed it . Travis fail is due to timeout in Mac build. I think you will have to re-trigger Mac build, after some time.

vg3095 · 2017-07-06T07:26:26Z

@wkerzendorf
Is there anything remaining to change in this PR ?

wkerzendorf · 2017-07-06T09:59:40Z

tardis/io/decay.py

+        return df 
+
+    def as_atomic_numbers(self, abundance, t, normalize=True):


thi should be called - merge_isotopes and should only perform the groupby operation.

wkerzendorf · 2017-07-06T10:00:28Z

tardis/io/decay.py

+        """
+        #Drop mass_number coloumn in isotopic_abundance dataframe
+        isotope_abundance = self.decay(t).reset_index(


use groupby to sum up over all mass numbers.

wkerzendorf · 2017-07-06T10:00:43Z

tardis/io/decay.py

+        isotope_abundance = self.decay(t).reset_index(
+            level='mass_number').drop('mass_number', axis=1)
+


then return here. The rest should happen somewhere else.

@wkerzendorf the rest means normalizing and returning as model abundance dataframe format should be moved to other separate function (as_atomic_numbers) and only groupby here(merge_isotopes)

wkerzendorf · 2017-07-06T10:01:11Z

tardis/model/base.py

@@ -73,6 +73,15 @@ def __init__(self, velocity, homologous_density, abundance, time_explosion,
        self.homologous_density = homologous_density
        self._abundance = abundance
        self.time_explosion = time_explosion
+
+        self.raw_abundance = self._abundance
+        if isotope_abundance is not None:


if it is None make an empty one.

@wkerzendorf I have made one , see line 82-84

You can use self.raw_isotope_abundance = isotope_abundance or pd.DataFrame()

wkerzendorf · 2017-07-06T10:01:46Z

tardis/model/base.py

-
+
+        if hasattr(config.model, 'isotope_abundance'):
+            isotope_abundance = config.model.isotope_abundance


no that's wrong - we have not decided on a config option and that should also come later!

yeganer · 2017-07-06T10:13:20Z

tardis/model/base.py

+        self.raw_abundance = self._abundance
+        if isotope_abundance is not None:
+            self.raw_isotope_abundance = isotope_abundance
+            self._abundance = self.raw_isotope_abundance.as_atomic_numbers(


I would add this line to the abundance property.

@yeganer If I put this line in under abundance property, then this line would be executed every time model.abundance is called , and I think it should only be called 1 time during initialization.

As your profiling showed, executing this for 100 elements takes less than 0.02 seconds. In my opinion it's okay to call it every time. especially as it should always reflect the current state of the model.

Currently tardis objects are not designed to be changed, nevertheless if one were to change time_explosion on the model, the isotopic abundances have to be recalculated. The easiest way to achieve this is by doing this (very fast) computation every time. I think you can count the number of accesses to model.abundance during a typical tardis run with one hand.

yeganer

I think after some small changes this PR is done.
Please add one commit dedicated to PEP8 issues currently in this file. There are tools for commandline and some editors also have plugins for this job.
I know you didn't write a lot of the code that contains style errors but I always try to fix all PEP8 issues whenever I edit a file :)

yeganer · 2017-07-06T13:37:30Z

tardis/io/decay.py

+        return self.groupby('atomic_number').sum()
+
+    def as_atomic_numbers(self, abundance, normalize=True):


I'd call this method something like merge_with or only merge as that reflects what this function does, merge Isotopes with a normal DataFrame.
Maybe we can call the other argument other so we can easily distinguish between self and other.

yeganer · 2017-07-06T13:41:02Z

tardis/io/decay.py

+        return df 
+
+    def merge_isotopes(self):


I'm not happy with the name of this function, maybe we should call it as_atoms? That's short and reflects that the output is only atoms instead of isotopes.

vg3095 · 2017-07-07T04:50:32Z

@wkerzendorf I think , in last meeting, I could not convey my message properly.

As of now as_atoms (Renamed from merge_isotopes , as per suggestion by @yeganer )-> Does only groupby to sum up over all mass numbers.
merge (Renamed from as_atomic_numbers)-> takes 2 data-frame , then adds and normalizes it .

I think, you confused with the names , as almost the names are reversed , and thought merge to be merge_isotopes.
If you want me to revert the names back,
(Make as_atoms to merge_isotopes and merge to as_atomic_numbers) , then a confirmation would be nice.

wkerzendorf · 2017-07-18T15:57:57Z

@vg3095 this is outdated right?

vg3095 · 2017-07-18T16:09:15Z

@wkerzendorf Yes, this is outdated. I will do a rebase

wkerzendorf · 2017-07-19T08:48:32Z

@vg3095 are you sure that is not in the codebase - as we merged the uniform case?

…rging

vg3095 · 2017-07-19T14:51:23Z

@wkerzendorf I have rebased it now. You can review it.

unoebauer · 2017-07-21T08:30:22Z

@wkerzendorf, @vg3095 - so, do we still need to merge this or not? I am confused since I thought that all the decay stuff has already been merged.

vg3095 · 2017-07-21T08:36:17Z

@wkerzendorf @unoebauer Yes, it needs to be merged. This is the only PR related to decay stuff. Rest all were related to config options for isotope.

…nfig

vg3095 changed the title ~~[TEP007] Decay and merge isotopic abundance dataframe~~ [TEP007] [WIP] Decay and merge isotopic abundance dataframe Jul 4, 2017

vg3095 changed the base branch from master to decay July 4, 2017 07:40

yeganer reviewed Jul 4, 2017

View reviewed changes

vg3095 force-pushed the model_decay branch 2 times, most recently from 8d1b968 to 9073eff Compare July 4, 2017 10:20

wkerzendorf reviewed Jul 4, 2017

View reviewed changes

yeganer reviewed Jul 4, 2017

View reviewed changes

wkerzendorf reviewed Jul 5, 2017

View reviewed changes

wkerzendorf reviewed Jul 6, 2017

View reviewed changes

yeganer reviewed Jul 6, 2017

View reviewed changes

vg3095 force-pushed the model_decay branch from 339c2bf to af1114f Compare July 19, 2017 12:25

vg3095 added 5 commits July 19, 2017 17:58

Add a decay method under Model class

7c517d1

Update Model decay method

018922e

Decay model if isotope_abundance parameter is present

f949d79

Fix for missing self positional argument

c399dae

Added test to check merge operation for abundance

f18d012

vg3095 added 11 commits July 19, 2017 17:58

Update merge operation for abundance

6d073b5

Move as_atomic_numbers method to IsotopeAbundances class

1ccaced

Added test for as_atomic_numbers method

ce10dbe

Move up isotope_abundance in initialization and use pd.add() while me…

b7e528a

…rging

Move decay of isotope_abundance under Model abundance property

9e122be

Use pd.groupby to merge isotopes

174d024

Change tests for decay

3ad7ebd

Fix typo in comment

67e9c37

Change name of func. in IsotopeAbundances class

007df67

PEP8 fixes

a5ed7a2

Use changed func. name in decay methods and its test

d9a5783

vg3095 force-pushed the model_decay branch from af1114f to d9a5783 Compare July 19, 2017 12:28

unoebauer approved these changes Jul 24, 2017

View reviewed changes

Make isotope_abundance instance of IsotopeAbundances in model.from_co…

53c2283

…nfig

vg3095 changed the title ~~[TEP007] [WIP] Decay and merge isotopic abundance dataframe~~ [TEP007] Decay and merge isotopic abundance dataframe Jul 28, 2017

wkerzendorf merged commit edc4ffa into tardis-sn:decay Aug 1, 2017

		self.time_explosion = time_explosion

		self.raw_abundance = self._abundance


		#Merge abundance dataframes
		modified_df = pd.concat([isotope_abundance, abundance])

		return df

		def as_atomic_numbers(self, abundance, t, normalize=True):

		isotope_abundance = self.decay(t).reset_index(
		level='mass_number').drop('mass_number', axis=1)



		if hasattr(config.model, 'isotope_abundance'):
		isotope_abundance = config.model.isotope_abundance

		return self.groupby('atomic_number').sum()

		def as_atomic_numbers(self, abundance, normalize=True):

[TEP007] Decay and merge isotopic abundance dataframe #757

[TEP007] Decay and merge isotopic abundance dataframe #757

Conversation

vg3095 commented Jul 4, 2017 • 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

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

vg3095 Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

vg3095 Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

yeganer commented Jul 4, 2017

vg3095 commented Jul 4, 2017 • edited Loading

vg3095 commented Jul 4, 2017

yeganer commented Jul 4, 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

vg3095 commented Jul 5, 2017

vg3095 commented Jul 6, 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vg3095 commented Jul 7, 2017 • edited Loading

wkerzendorf commented Jul 18, 2017

vg3095 commented Jul 18, 2017 • edited Loading

wkerzendorf commented Jul 19, 2017

vg3095 commented Jul 19, 2017

unoebauer commented Jul 21, 2017

vg3095 commented Jul 21, 2017

vg3095 commented Jul 4, 2017 •

edited

Loading

vg3095 Jul 5, 2017 •

edited

Loading

vg3095 Jul 5, 2017 •

edited

Loading

vg3095 commented Jul 4, 2017 •

edited

Loading

vg3095 commented Jul 7, 2017 •

edited

Loading

vg3095 commented Jul 18, 2017 •

edited

Loading