-
Notifications
You must be signed in to change notification settings - Fork 15
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
[ENH] PC algorithm with updated example using dowhy.gcm for the SCM module #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this so long as my requests are addressed.
Also, can you double check that you have tests in place that will protect this code against pending refactors to the context object?
Codecov Report
@@ Coverage Diff @@
## main #30 +/- ##
===========================================
+ Coverage 68.04% 78.75% +10.71%
===========================================
Files 13 14 +1
Lines 679 805 +126
Branches 126 157 +31
===========================================
+ Hits 462 634 +172
+ Misses 175 118 -57
- Partials 42 53 +11
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
The context object is only accessed through its class properties rn and then the instantiations are used in the unit tests. They are used in conjunction w/ the PC algo, so in principle changes to the context that break PC algo, will be detected by those tests. I've addressed most of your comments, except #30 (comment). In addition, some misc. topics:
|
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get this merged if there are no further concerns
Signed-off-by: Adam Li <adam2392@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we can address include/exclude edge issues elsewhere.
Signed-off-by: Adam Li <adam2392@gmail.com>
Sorry can you re-approve? Fixing the merge conflicts inside changelog and conf.py and it disables your previous approval :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapproving
…odule (#30) * Adding example demonstrating usage of PC algorithm on simulated SCM data * Add PC algorithm basic implementation Signed-off-by: Adam Li <adam2392@gmail.com> Signed-off-by: Chris Trevino <darthtrevino@gmail.com>
Fixes #18
Closes: #10
Changes proposed in this pull request:
Before submitting
section of the
CONTRIBUTING
docs.Writing docstrings section of the
CONTRIBUTING
docs.After submitting