Skip to content
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] Add cmfgen2tardis script #771

Merged
merged 13 commits into from Aug 2, 2017
Merged

Conversation

vg3095
Copy link
Contributor

@vg3095 vg3095 commented Jul 20, 2017

This script takes a CMFGEN file as an input , and an output path to save the converted CSV file in new TARDIS file format.
An example of input CMFGEN file (Link) and its converted CSV file (Link)
Example, for how to use-
Format - cmfgen2tardis /path/to/input_file path/to/output/
cmfgen2tardis tardis_example/DDC15_SN_HYDRO_DATA_0.976d tardis_example/


  • Now velocity, density, electron_density and temperature are also saved.

  • Values are stored in reverse order, to match how Tardis stores things. For ex - velocity values in Tardis are stored in increasing order, while in Cmfgen files , it is in decreasing order .

@@ -0,0 +1 @@
from scripts.cmfgen2tardis import *
Copy link
Contributor Author

@vg3095 vg3095 Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wkerzendorf @yeganer When I leave __init__.py file empty, travis fails, because it skips this file.
(warning: build_scripts: scripts/__init__.py is an empty file (skipping)). Travis Report.
Can this be avoided ?.

return df.transpose()


def parse_file():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it works that way. look at the link I sent you

Copy link
Contributor Author

@vg3095 vg3095 Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wkerzendorf I think, you are saying to create a __main__.py file under scripts, as in the link you sent. If we do , as in the link you sent, we may be able to do something like this -

tardis_scripts cmfgen2tardis --input_file=/path/file --output_file=/tmp/
tardis_scripts get_spectrum --gdb

So, first argument tardis_scripts will be fixed, meaning we want to run scripts, second argument will be the script we want to run, and rest will be arguments required for individual scripts.

By my current approach
cmfgen2tardis /path/file /tmp/
will work, and for this we do not require a __main__.py file.Stackoverflow

So do you want the 1st way or 2nd way ?

@unoebauer
Copy link
Contributor

Would be good to have a more detailed description of the scope of this PR... Is it taking CMFGEN config files and then generating appropriate TARDIS inputs or is it supposed to do something else?

@vg3095
Copy link
Contributor Author

vg3095 commented Jul 21, 2017

@unoebauer I have updated the description for PR now.

@wkerzendorf
Copy link
Member

@vg3095 I don't think these should live within scripts outside of the package. I think you also misunderstood the entrypoints idea. Essentially it will generate a script based on calling a "main" function that takes args. so you wrote it like a script but that is not the right approach. Write is as a function that takes args.

@wkerzendorf
Copy link
Member

@vg3095 can you also link a CMFGEN model?

@vg3095
Copy link
Contributor Author

vg3095 commented Jul 21, 2017

@wkerzendorf I have now added a CMFGEN file in the description

@wkerzendorf
Copy link
Member

@vg3095 once this works - we also need to read the velocity, temperature, atom density and electron density.

df.to_csv(os.path.join(args.output_path, save_name), index=False, sep=' ')


def main():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they entry point actually gives args to the function. So we might want to use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wkerzendorf See these links

setuptools console_scripts entry point wants a function of no arguments.

  • Blog. Here also entry point function is used without args.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am missing something very obvious here, which I can't see, then please point it out.
I also saw runner script , here run method is used as an entry point function and is used without passing args.

@wkerzendorf
Copy link
Member

@vg3095 actually, I think its close. Let's do some documentation and then I think we can merge and work on getting the velocity and other quantities in there.

@vg3095
Copy link
Contributor Author

vg3095 commented Jul 24, 2017

@wkerzendorf I have now added doc for the script.

else:
break

df.loc[element_symbol] = abundances
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wkerzendorf As you said in PR #772 , if we are reversing velocity, then all other columns should also be reversed, so that means , while saving abundances from CMFGEN file, we should also reverse them, -- right?

1. Now velocity,density,electron_density and temperature are also saved as DAT file.
2. Since, we are reversing velocity, so reversing all columns along with it.
@vg3095
Copy link
Contributor Author

vg3095 commented Jul 28, 2017

  • Now velocity, density, electron_density and temperature are also saved as DAT file.
  • Since, we are reversing velocity, so reversing all columns along with it.

@vg3095
Copy link
Contributor Author

vg3095 commented Jul 31, 2017

Changed script, to store all quantities in a single CSV file. Example of a converted file --> Link

@vg3095 vg3095 closed this Aug 2, 2017
@vg3095 vg3095 reopened this Aug 2, 2017
@wkerzendorf wkerzendorf merged commit f6d00b7 into tardis-sn:decay Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants