-
-
Notifications
You must be signed in to change notification settings - Fork 422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TEP007] CMFGEN density parser #772
Conversation
To resolve conflict of same attribute names in IonNumberDensity
I have overwrited |
n_electron_iterations = 0 | ||
|
||
while True: | ||
if self._electron_densities is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe make ElectronDensities an adiitional module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkerzendorf Okay, If I make a ElectronDensities
module like this under plasma_input.py
class ElectronDensities(ArrayInput):
outputs = ('electron_densities',)
-
First of all, If we are not supplying
electron_densities
from CMFGEN file (normal cases), then how will I set this value ?
The Logic I saw inIonNumberDensity
, calculates bothelectron_densities
andion_number_density
. And if I make a separate module
for it, then I will have to removeelectron_densities
from the output list ofIonNumberDensity
, because then it will giver error while
resolving variables(_resolve_update_list
method) -
And then I think , it will resulting in changing some tests/fixtures which are dependent on electron_density, if above behaviour is changed.
If you could be more descriptive, it will be nice for me 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe what you did is fine. I was hoping to make it even nicer - but maybe for now it's fine.
Read a file, where first line is for time_explosion, second line is header row containing velocity,density,electron_densities, and temperaturer
If electron_densities is read from a file, then it will directly use these values, without calculating it.
tardis/io/model_reader.py
Outdated
def read_cmfgen_density(fname): | ||
|
||
df = pd.read_csv(fname, comment='#', delimiter='\s+', skiprows=1) | ||
velocity = (df['Velocity'].values[::-1] * u.km / u.s).to('cm/s') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkerzendorf For CMFGEN files , I think the convention for velocity
is different . For ex-
-
In CMFGEN files velocity values are in
decreasing
order. CMFGEN file
6.9999926E+04 6.9999184E+04 ........ 8.7744269E+02 8.7166905E+02
-
In TARDIS files, velocity values are in
ascending
order. TARDIS file
9000.0000 10500.000 ......................... 21000.000 22500.000
-
So, when I did not reverse velocity array here(for CMFGEN case), it was not parsing due to this part of code( because I think the convention for velocity is different for CMFGEN case)
v_inner = velocity[:-1]
v_outer = velocity[1:]
invalid_volume_mask = (v_outer - v_inner) <= 0
- So, I have reversed it here, so that CMFGEN velocity values can be in ascending order as well
I don't know , if I am right, so can you check this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vg3095 if you reverse the velocity - you have to reverse all the other columns as well. They all belong together.
For CMFGEN files , the convention for velocity is different than tardis format. In CMFGEN files velocity values are in decreasing order, while in In TARDIS files, velocity values are in ascending order. So, reversing other columns as well.
These tests ensure that if electron_densities and temperature are read from (tardis_model/cmfgen) density file, they are used directly , rather than being calculated.
I have now added unit tests. |
1. While saving cmfgen files to tardis_model file format using cmfgen script, all columns will be stored in reverse order, so that there is no need to reverse it in read_cmfgen_density func. 2. Changes in values of unit tests to reflect for the point above.
While saving |
tardis/io/model_reader.py
Outdated
@@ -235,6 +244,20 @@ def read_artis_density(fname): | |||
return time_of_model, velocity, mean_density | |||
|
|||
|
|||
def read_cmfgen_density(fname): | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation please
@@ -0,0 +1,13 @@ | |||
0.976 day | |||
Index Velocity Densities ElectronDensities Temperature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please save without index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and everything lower case and singular - and with underscore.
1. Now first 4 columns are related to velocity,density,electron_densities,temperature. Abundances begin from 5th column onwards. 2. First line of abundances is ignored, same as in rest of cases for other abundance formats.
1. A common CSV file, containing quantities related to both abundances and density parser. 2. Changes in unit tests config files to reflect the same.
Since we are now using a common file for both
I have now made a single CSV file for abundances, density, etc. Also added docstrings wherever required. |
@@ -0,0 +1,12 @@ | |||
0.976 day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a t_0:
before that. but other than that - very nice!!
@@ -0,0 +1,12 @@ | |||
0.976 day | |||
velocity densities electron_densities temperature C O Mg Si Ni56 Ni58 | |||
871.66905 4.2537191E-09 259538070000000 7.6395577 0 0 0 0.6 0.4 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
electron_densities - should be float.
and add one more line with units for the different quantities. with 1 for dimensionless units (such as abundance fractions)>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the temperatures here are 10^4 k
@@ -0,0 +1,12 @@ | |||
0.976 day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please write as t_0: ...
Now there is a line after header row, which contain quantites. It reads that line and convert attributes according to that quantity
@@ -0,0 +1,13 @@ | |||
t0: 0.976 day | |||
velocity temperature densities electron_densities C O Mg Si Ni56 Ni58 | |||
km/s K gm/cm^3 /cm^3 1 1 1 1 1 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gm/cm^3
can become g/cm^3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkerzendorf I have updated it now
@tardis-sn/tardis-core I'm signing off - can someone else look over this please! |
A parser to read velocity , density, electron_density from a CMFGEN file.
Example -
Notebook
CMFGEN file
Converted CMFGEN file