-
Notifications
You must be signed in to change notification settings - Fork 17
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
CORRECTED PA/MA Pipeline Update and GrFN2WD Translation #344
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 see some checks are failing, but but besides that things look good to me.
If you need to merge before all tests are passing, that's fine, your call.
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.
@pauldhein I've left a few comments on the PR, please take a look at them when you get a chance.
delphi/GrFN/networks.py
Outdated
self.inputs = [ | ||
n | ||
for n, d in self.in_degree() | ||
if d == 0 and self.nodes[n]["type"] == "variable" | ||
] | ||
self.output_node = self.outputs[-1] | ||
# self.output_node = self.outputs[-1] |
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.
Can we get rid of this commented-out line?
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.
Done!
delphi/GrFN/networks.py
Outdated
) | ||
|
||
# {"file_name": f"{stem}.py"}, # HACK |
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.
Is this commented-out line something we can remove as well?
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.
Removed.
@@ -0,0 +1,2582 @@ | |||
{ |
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.
Is this a file that can be automatically generated? If so, we should remove it.
@@ -0,0 +1,259 @@ | |||
import math |
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.
Similarly for this file - can it be automatically generated? If so, let's remove it.
@@ -0,0 +1,160 @@ | |||
{ |
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.
Same question (automatically generated?)
delphi/translators/for2py/f2grfn.py
Outdated
Returns: | ||
Element Tree (ET) Object: An object of generated rectified | ||
XML. | ||
""" |
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.
The extra indentation here in the docstrings makes sphinx-apidoc complain. This was fixed in #333, but the fix doesn't seem to have propagated to this branch. Perhaps you can try doing git cherry-pick a22e0d506d2bb51b20c027e1a7748eece9ffbf12
?
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 used the cherry-pick
method and was indeed able to revert the commenting style to the correct solution. @adarshp please let me know if I missed one of the docstrings in the new version
delphi/translators/for2py/rectify.py
Outdated
<file> | ||
</file> | ||
""" | ||
This function handles cleaning up the XML elementss |
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.
Again, docstring indentations.
tests/aske/test_GrFN.py
Outdated
CAG.draw('SIR-Gillespie_ms--CAG.pdf', prog='dot') | ||
|
||
|
||
# def test_petasce_torch_execution(): |
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.
Can we delete this commented-out code?
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.
My mistake, I still want this test in the future so I'll use the pytest skip fixture as opposed to commenting out the code.
Also, @pauldhein if you get around to merging before me, please make sure to squash the commits before merging! |
Codecov Report
@@ Coverage Diff @@
## master #344 +/- ##
=========================================
+ Coverage 67.96% 69.07% +1.1%
=========================================
Files 59 60 +1
Lines 8342 8990 +648
=========================================
+ Hits 5670 6210 +540
- Misses 2672 2780 +108
Continue to review full report at Codecov.
|
* Fixed unexpected indentations within the docstrings for f2grfn.py * Fixed additonal docstring issue for f2grfn.py * Fixed docstring issues for format.py * Fixed docstring issues for get_comments.py * Fixed docstring issues for preprocessor.py * Fixed docstring issues for pyTranslate.py * Fixed docstring issues for static_save.py * Fixed docstring issues for strings.py * Fixed docstring issues for syntax.py * Fixed docstring issues for translate.py * Partially fixed docstring issues for type_ext.py * Fixed all docstring issues for rectify.py * Fixed all docstring issues
This new branch includes all of the material from #256 that is needed to support the SIR models and collaborative demo with GTRI and Galois. The reason for this new branch is to avoid merge errors with master. This PR should solve the problems found in previous PRs.