-
-
Notifications
You must be signed in to change notification settings - Fork 409
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] Isotope stratified file support #764
Conversation
Travis fail is due to Mac build timeout. |
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.
I am wondering whether we can simplify the configuration a bit. In particular, I am thinking of the following changes:
- Remove the second line in the stratified file (containing
E
andI
s); it is clear from the header whether the column contains abundances of elements or isotopes; if the identifier in the header contains a number it is an isotope, otherwise an element - remove the additional config identifier
filetype: isotopes
; we could achieve legacy support by checking for a header line; if it is present we assume the new csv format, if not we assume the old format.
Thoughts on this, @vg3095, @wkerzendorf, @yeganer ?
|
tardis/io/model_reader.py
Outdated
@@ -85,19 +85,66 @@ def read_abundances_file(abundance_filename, abundance_filetype, | |||
""" | |||
|
|||
file_parsers = {'simple_ascii': read_simple_ascii_abundances, | |||
'artis': read_simple_ascii_abundances} | |||
'artis': read_simple_ascii_abundances, | |||
'isotopes': read_simple_isotope_abundances} |
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.
I would actually call this tardis_model. I think this can be some sort of default model. where you specify things in this way
9043a33
to
b1799e7
Compare
I have removed the 2nd header line('E' or 'I') and changed filetype name to |
#Assign header row | ||
df.columns = [nucname.name(i) | ||
for i in range(1, df.shape[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.
nucname.name(i) will throw an error, if somebody a wrong specifies an element or isotope identifier, right?
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.
Yes, it will throw a Runtime Error
with the wrong name of element/isotope. Ex- RuntimeError: Not a Nuclide! XYZ --> 0
, where XYZ
is wrongly specified element.
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.
Excellent
abundance_filename) | ||
else: | ||
index, abundances = file_parsers[abundance_filetype]( | ||
abundance_filename) |
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.
Can we improve here? Something like
index, abundances, isotope_abundance = file_parser[abundance-filetype]
such that we don't need the if clause?
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.
If, read_simple_ascii_abundances
function returns an extra None
(instead of (index,abundance)
, returning index,abundance,None
)) , then we can remove this. But I don`t think , its a good idea.
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.
Ok, fine with me
@vg3095 I overlooked that before - but we definitely need tests and documentation for all of this. |
b1799e7
to
c565565
Compare
@wkerzendorf I have now updated this PR with tests. |
@vg3095 in your examples - you should try to make the abundances add up to one 😉 |
tardis/io/model_reader.py
Outdated
@@ -256,3 +264,31 @@ def read_simple_ascii_abundances(fname): | |||
abundances = pd.DataFrame(data[1:,1:].transpose(), index=np.arange(1, data.shape[1])) | |||
|
|||
return index, abundances | |||
|
|||
|
|||
def read_simple_isotope_abundances(fname, delimiter=','): |
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 default delimiter should be a whitespace. this is a special option in read_csv
.
@@ -0,0 +1,11 @@ | |||
C,Mg,Si,Ni56,Ni58 |
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.
default delimiter should be whitespace.
@wkerzendorf I have now updated csv files |
@vg3095 can you rebase this. @tardis-sn/tardis-core looks good from my side - can someone else have a look? |
abundance_filename) | ||
else: | ||
index, abundances = file_parsers[abundance_filetype]( | ||
abundance_filename) |
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.
Ok, fine with me
#Assign header row | ||
df.columns = [nucname.name(i) | ||
for i in range(1, df.shape[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.
Excellent
This PR is related to adding support for reading
stratified abundances with isotopes
. This is rebased over PR #762 (Isotope uniform config)I have added 2 functions -
read_simple_isotope_abundances
: To read isotope abundances file in new formatconvert_abundances_format
: To convert old abundances file format to new oneFor sample seeing output:
Notebook
iso_abund.csv
Config