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

Fix issue with custom_composition reader #843

Merged
merged 10 commits into from
Jul 23, 2018

Conversation

unoebauer
Copy link
Contributor

@unoebauer unoebauer commented Jul 20, 2018

Currently, the custom_composition reader is not properly working since it expected an additional column containing the cell index, similar to the file struture for the simple_ascii reader.

This PR addresses this issue by re-inserting the cell index as the first column into the custom_composition files.

  • fix read_csv_isotope_abundances and read_cmfgen_composition
  • update tests and reference files
  • update documentation

@pfreddy
Copy link
Contributor

pfreddy commented Jul 20, 2018

I changed the line 405 in model_reader.py, it still gives the error message.

@unoebauer
Copy link
Contributor Author

@pfreddy, @wkerzendorf - I think, the PR is ready. It passes our testing suite offline and it create data frames with the correct shape from the test_csv.dat file from @pfreddy. Whenever Tardis is happy and someone reviewed it, we should consider merging.

@unoebauer
Copy link
Contributor Author

Travis is happy!

df = pd.read_csv(fname, comment='#',
delimiter=delimiter, skiprows=skip_rows)
sep=delimiter, skiprows=skip_rows, index_col=0)
Copy link
Member

Choose a reason for hiding this comment

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

let's rename it into skiprows to be in line with pandas

@wkerzendorf
Copy link
Member

@pfreddy is checking your PR.

@unoebauer
Copy link
Contributor Author

Ok, I've revisited the issue and found some indexing problems in the parser which are hopefully fixed now. On my laptop, I can run your setup @pfreddy without problems. Also the testing suite runs through. If Travis is happy, I'll address your stylistic suggestion, @wkerzendorf .

@wkerzendorf wkerzendorf merged commit 78aa633 into tardis-sn:master Jul 23, 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.

3 participants