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

Fixes #830 #832

Merged
merged 9 commits into from Jun 25, 2018
Merged

Fixes #830 #832

merged 9 commits into from Jun 25, 2018

Conversation

unoebauer
Copy link
Contributor

@unoebauer unoebauer commented Jun 19, 2018

This PR implements changes to the parsers of the various composition files to address #830.

@unoebauer
Copy link
Contributor Author

The test_model_reader.py does fail. Judging from the setup of the tests, the modified parser was indeed intended for CMFGEN setups.

Before proceeding, we should discuss our options, @wkerzendorf

@unoebauer
Copy link
Contributor Author

I would suggest the following:

def read_density_file(filename, filetype):
    file_parsers = {'artis': read_artis_density,
                    'simple_ascii': read_simple_ascii_density,
                    'cmfgen_model': read_cmfgen_density}
    ...
def read_abundances_file(abundance_filename, abundance_filetype,
    file_parsers = {'simple_ascii': read_simple_ascii_abundances,
                    'artis': read_simple_ascii_abundances,
                    'cmfgen_model': read_cmfgen_composition,
                    'simple_csv': read_csv_composition}
def read_csv_isotope_abundance(self, delimiter="\+s", skip_columns=0):
    ...
    for element_symbol_string in df.index[skip_columns:]:
        ...
def read_cmfgen_composition(self, delimiter="\+s"):
    return self.read_csv_isotope_abundance(delimiter=delimiter, skip_columns=4)

def read_csv_composition(self, delimiter="\+s"):
    return self.read_csv_isotope_abundance(delimiter=delimiter, skip_columns=0)

What do you think, @wkerzendorf ?

@wkerzendorf
Copy link
Member

I think we should rename the cmfgen thing. I think it should be standardized input that is useful for us and we can make something that converts it to our model. I'd like to chat on that on skype

@unoebauer
Copy link
Contributor Author

I still think that for now we should go with a quick fix on the lines that I've suggested. The CMFGEN format is quite specific. And standardizing the input is an undertaking that goes beyond the original scope of this PR.

@wkerzendorf
Copy link
Member

@unoebauer I think we misunderstand each other - the main thing I'm suggesting is to drop the CMFgen name - as it is not true at the moment. I'll try to catch you online to discuss.

@unoebauer
Copy link
Contributor Author

The tests should pass now (checked in offline on my laptop). If Travis is happy, I'd say that this PR is ready for merging. What do you think, @wkerzendorf ?

@wkerzendorf
Copy link
Member

@unoebauer very nice PR (feature; docu + tests - dass ich das noch erleben darf). Doesn't pass - you need to add the file in the package template

@unoebauer
Copy link
Contributor Author

unoebauer commented Jun 22, 2018

I see, I forgot to update the tests in test_tardis_model_density_config.py. I will do that soon.

@unoebauer
Copy link
Contributor Author

Fixed a few outstanding issues - the complete test suite now passes locally. Hopefully Travis is happy now as well.

@wkerzendorf wkerzendorf merged commit a8e6aa5 into tardis-sn:master Jun 25, 2018
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

2 participants