tardis-sn / tardis

TARDIS - Temperature And Radiative Diffusion In Supernovae
https://tardis-sn.github.io/tardis
201 stars 405 forks source link

Tardis cannot parse elements which are not formatted starting with capitalized characters #2002

Open Rodot- opened 2 years ago

Rodot- commented 2 years ago

Describe the bug When elements are provided to tardis in CSVY data files, elements provided without the proper capitalization will not always be interpreted correctly by the radioactive decay module.

To Reproduce Run a tardis csvy model containing some abundance of ni56 labeled as ni56 exactly.

Screenshots

System

Additional context

jayantbhakar commented 2 years ago

Here's a csvy model I ran Screenshot from 2022-05-05 15-50-35 Here when I changed Ni56 to ni56 in both the lines (i.e line number 24 and 28), it ran without any problem. It shows error only when the case sensitive names don't match.

jayantbhakar commented 2 years ago

What do we want here? It should not match the case in both the lines?

jayantbhakar commented 2 years ago

I am not sure of the assigned option here. Just wanted to know if an issue is assigned to anyone, can anyone else still try to resolve that issue.

lemointm commented 2 years ago

Hi @jayantbhakar, can you please share a photo of the error you're getting when Ni56 is uncapitalized? I ran your csvy model and was not able to replicate the error.

lemointm commented 2 years ago

On my machine TARDIS ignores the nickel when uncapitalized, and runs normally when capitalized as expected

jayantbhakar commented 2 years ago

No, the model I showed above won't show an error. This is what I wanted to point out. In the csvy model above, if the name( element name i.e. ni56) on line 24 doesn't match with the name on line 28 (including the capitalization). In the above model it is not showing an error because of this reason. If we change the "ni56" on line 24 to "Ni56" it will show an error. Hope, I am able to communicate well.

lemointm commented 2 years ago

Are you getting the error "CSVY columns exist without field descriptions"? I mismatched capitalization on lines 24 and 28 and got that error. I think that is due to TARDIS expecting the data fields you enter on line 28 to match the data fields entered on lines 7-25(although I am not very well acquainted with CSVY files)

jayantbhakar commented 2 years ago

Yes, that's right.

jayantbhakar commented 2 years ago

So what do we want to achieve now? TARDIS should not match the capitalization?

lemointm commented 2 years ago

Does your model run when lines 24 and 28 both have Ni56 capitalized? If it does I think the issue can be marked resolved, since that is the intended formatting of the csvy model as far as I can tell (or it is at least what the TARDIS model reader expects)

jayantbhakar commented 2 years ago

Yes, it doesn't show any error when Ni56 is capitalized in both the lines. But in the issue description, it is mentioned that ni56 should be written as "ni56", which can be achieved as shown in the above image I sent.

lemointm commented 2 years ago

Upon further inspection, the way TARDIS parses isotope names requires that the names be capitalized like they are by convention (i.e. Ni as opposed to ni). This could be changed to add the ability to input uncapitalized nuclides but it would be troublesome (TARDIS does not do the nuclide parsing itself) so I would recommend that you make sure that the isotopes are just capitalized when they're put into the csvy file as an easy fix

jayantbhakar commented 2 years ago

For this, we can keep the element name in line 24 capitalized.

jayantbhakar commented 2 years ago

I didn't understand when you said it will be troublesome to input uncapitalized nuclides. Since if we want any isotope to be uncapitalized we just have to change only the first letter at just two places in the csvy file.

lemointm commented 2 years ago

The function to check whether or not the isotope is a actual isotope that exists in the atomic dataset TARDIS uses expects the isotope or element names to be in the conventional form (Ni56 or Ni-56, for example). Thus it's just the way it has to be inputted for the model to be read properly. Like you said too, the user just has to capitalize the two places to make it run properly, I can add that to the documentation perhaps to make it more clear

lemointm commented 2 years ago

The doumentation for the csvy model could certainly be updated to make the specification of abundances more clear

jayantbhakar commented 2 years ago

Okay, now I get that. Yes, adding it to the documentation will be better.

Rodot- commented 2 years ago

Upon further inspection, the way TARDIS parses isotope names requires that the names be capitalized like they are by convention (i.e. Ni as opposed to ni). This could be changed to add the ability to input uncapitalized nuclides but it would be troublesome (TARDIS does not do the nuclide parsing itself) so I would recommend that you make sure that the isotopes are just capitalized when they're put into the csvy file as an easy fix

We would like this changed such that the tables work with both the capitalized and uncapitalized names. For now, let's make it such that both will work so long as they are consistent.

Rodot- commented 2 years ago

This issue is important because it breaks backwards compatibility currently with older abundance files

lemointm commented 2 years ago

I just discovered this is an issue only with nickel - due to the fact that radioactivedecay thinks the lowercase 'n' in 'ni' is a metastable state flag. That's why it works when the 'n' is capitalized because it recognizes it as a element. I will try to get this fixed, however it should only be a problem with nickel (which admittedly is a big issue since nickel is so important in Type Ia supernova but it's good to know at least that this is the root of the issue)

jayantbhakar commented 2 years ago

So, are we working on changing the code, so that it ignores the uppercase or lowercase isotope name?

lemointm commented 2 years ago

Hi @jayantbhakar, we just published a new release of radioactivedecay. Try updating it and rerunning your .csvy model - it should work now even when uncapitalized