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

PR - Include Source generation script/Change domain #25

Merged
merged 4 commits into from
Sep 28, 2017
Merged

PR - Include Source generation script/Change domain #25

merged 4 commits into from
Sep 28, 2017

Conversation

Arvinds-ds
Copy link
Contributor

  1. Include observations.r and observation.tests.rgeneration scripts inobservations/src`
  2. Change domain for data files and rst files to a new URL
  3. .gitignore included csv file and it ignored the datasets.csv file which was required for generation

@dustinvtran
Copy link
Member

Is this PR ready to review? Looks like it still stores all the rst files.

@Arvinds-ds
Copy link
Contributor Author

My bad. Forgot to remove the doc folder. Pls see latest commit

@dustinvtran
Copy link
Member

Looks great! One comment: Travis seems to take >30 minutes and is timing out, whereas in #20 it only took 12 minutes. In principle, the only change is the download server. Is this slowdown really because of the download?

@Arvinds-ds
Copy link
Contributor Author

I noticed that. I think because Travis and Github are well integrated, it is fast. The flip side is that I was testing it locally when I was @ Columbia and it took < 200 secs and all tests passed. I am ssuming Travis has some restrictions on downloading data from external servers.

@Arvinds-ds
Copy link
Contributor Author

Also I notice randon connection errors in the travis build log for e.g. affairs.py where as on my local machine it works..
image

Also, when I reran the tests again locally, it passed
image

@dustinvtran
Copy link
Member

dustinvtran commented Sep 27, 2017

Got it. This sounds like a pervasive issue which is relevant to this PR but is more about #23 and how to handle Travis downloading many data sets. I propose merging the PR as is and leaving that issue open. Thoughts?

@Arvinds-ds
Copy link
Contributor Author

Agree. For review purposes, Would request you to clone the repo and run tests locally and verify. It works at my end, but need you to validate it..

@dustinvtran
Copy link
Member

Done!

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.

2 participants