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

Octo 1616 no experimentdata #134

Merged
merged 51 commits into from
Jul 26, 2017

Conversation

gbordyugov
Copy link
Contributor

  • PLEASE DON'T MERGE YET *

@coveralls
Copy link

coveralls commented Jul 20, 2017

Coverage Status

Coverage increased (+2.8%) to 82.062% when pulling 4b9484e on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.9%) to 82.133% when pulling 63d9a46 on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+2.9%) to 82.133% when pulling 63d9a46 on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

}

if not method in method_table:
if not method in workerTable:
Copy link
Contributor

Choose a reason for hiding this comment

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

if method not in workerTable may be a bit more readable.

self.assertEqual (res['n_x'], 1000)
self.assertEqual (res['n_y'], 1000)
self.assertAlmostEqual (res['mu_x'], -0.045256707490195384)
self.assertAlmostEqual (res['mu_y'], 0.11361694031616358)
Copy link
Contributor

Choose a reason for hiding this comment

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

to my opinion this structure is rather difficult to maintain if there are some changes (you need to keep in mind spaces all the time).
https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it would be easier if we could agree on a specific style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that writing this might be a bit cumbersome, but reading is much easier - and we tend to read more often than write.

Copy link
Contributor

@mkolarek mkolarek left a comment

Choose a reason for hiding this comment

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

looks great, we could potentially merge these changes with the dev branch, but not with master. I believe this would ease further collaboration without breaking and outside dependencies (yet).

@@ -159,6 +165,8 @@ def get_or_compile_stan_model(model_file, distribution):
pickle.dump(sm, f)
return sm

cacheSamplingResults = False
samplingResults = {} # memoized sampling results
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for PEP8


res += '\n {:d} variants: {}'.format(len(variants),
Copy link
Contributor

Choose a reason for hiding this comment

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

"res" is unresolved variable, there are no declaration (or was commented?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's part of the old (removed) code, hence in red.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it! thanks!


res += '\n {:d} variants: {}'.format(len(variants),
Copy link
Contributor

@daryadedik daryadedik Jul 21, 2017

Choose a reason for hiding this comment

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

Ah, here the res was commented and now 'res' has no declaration. Comment before actually was about that, I just missed the place.

@gbordyugov
Copy link
Contributor Author

variable names style change from camelCase to snake_case

@coveralls
Copy link

coveralls commented Jul 24, 2017

Coverage Status

Coverage increased (+2.9%) to 82.133% when pulling 943442f on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

@coveralls
Copy link

coveralls commented Jul 24, 2017

Coverage Status

Coverage increased (+2.9%) to 82.133% when pulling c3507be on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

@coveralls
Copy link

coveralls commented Jul 24, 2017

Coverage Status

Coverage increased (+2.9%) to 82.133% when pulling 8ccc1af on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

…group_sequential_delta in expan/core/early_stoppy.py
@coveralls
Copy link

coveralls commented Jul 26, 2017

Coverage Status

Coverage increased (+2.8%) to 82.072% when pulling c8a330d on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

@coveralls
Copy link

coveralls commented Jul 26, 2017

Coverage Status

Coverage increased (+2.8%) to 82.072% when pulling e5a4367 on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

@coveralls
Copy link

coveralls commented Jul 26, 2017

Coverage Status

Coverage increased (+2.8%) to 82.072% when pulling bb1e087 on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

@coveralls
Copy link

coveralls commented Jul 26, 2017

Coverage Status

Coverage increased (+2.9%) to 82.166% when pulling 1f6f080 on gbordyugov:OCTO-1616-no-experimentdata into dfbf3ed on zalando:dev.

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.

5 participants