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

Preserve path to the file when rendering model #3186

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

LysSanzMoreta
Copy link
Contributor

No description provided.

@@ -556,7 +556,7 @@ def render_model(
model: Callable,
model_args: Optional[Union[tuple, List[tuple]]] = None,
model_kwargs: Optional[Union[dict, List[dict]]] = None,
filename: Optional[str] = None,
filepath: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Could you keep the signature filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, is it ok now? :)

fehiepsi
fehiepsi previously approved these changes Mar 2, 2023
Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Thanks, @LysSanzMoreta !

suffix = filename.suffix[1:] # remove leading period from suffix
graph.render(filename.stem, view=False, cleanup=True, format=suffix)
suffix = Path(filename).suffix[1:] # remove leading period from suffix
graph.render(os.path.splitext(filename)[0], view=False, cleanup=True, format=suffix)
Copy link
Member

Choose a reason for hiding this comment

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

it seems that you need to import os

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I re-added it. Not sure why it was not there

Copy link
Member

Choose a reason for hiding this comment

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

could you run make format to fix the lint issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fehiepsi Ops, sorry I am a little beginner. I am not sure what make format means here. Is it another package?

I did run pylint over the script with this output:

 ************* Module pyro.infer.inspect
inspect.py:1:0: C0114: Missing module docstring (missing-module-docstring)
inspect.py:15:0: R0402: Use 'from pyro import poutine' instead (consider-using-from-import)
inspect.py:26:0: C0116: Missing function or method docstring (missing-function-docstring)
inspect.py:37:4: C0103: Variable name "fn" doesn't conform to snake_case naming style (invalid-name)
inspect.py:39:8: C0103: Variable name "fn" doesn't conform to snake_case naming style (invalid-name)
inspect.py:46:0: C0115: Missing class docstring (missing-class-docstring)
inspect.py:61:0: R0914: Too many local variables (24/15) (too-many-locals)
inspect.py:183:20: E1102: poutine.trace is not callable (not-callable)
inspect.py:204:12: C0103: Variable name "u" doesn't conform to snake_case naming style (invalid-name)
inspect.py:206:16: C0103: Variable name "d" doesn't conform to snake_case naming style (invalid-name)
inspect.py:211:8: C0103: Variable name "d" doesn't conform to snake_case naming style (invalid-name)
inspect.py:212:12: C0103: Variable name "u" doesn't conform to snake_case naming style (invalid-name)
inspect.py:212:15: C0103: Variable name "p" doesn't conform to snake_case naming style (invalid-name)
inspect.py:221:8: C0103: Variable name "d" doesn't conform to snake_case naming style (invalid-name)
inspect.py:223:12: C0103: Variable name "u1" doesn't conform to snake_case naming style (invalid-name)
inspect.py:223:16: C0103: Variable name "p1" doesn't conform to snake_case naming style (invalid-name)
inspect.py:224:16: C0103: Variable name "u2" doesn't conform to snake_case naming style (invalid-name)
inspect.py:224:20: C0103: Variable name "p2" doesn't conform to snake_case naming style (invalid-name)
inspect.py:61:0: R0912: Too many branches (13/12) (too-many-branches)
inspect.py:285:20: E1102: poutine.trace is not callable (not-callable)
inspect.py:323:12: C0103: Variable name "p" doesn't conform to snake_case naming style (invalid-name)
inspect.py:323:15: C0103: Variable name "pv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:324:12: C0103: Variable name "pv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:325:16: C0103: Variable name "q" doesn't conform to snake_case naming style (invalid-name)
inspect.py:325:19: C0103: Variable name "qv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:326:16: C0103: Variable name "qv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:323:12: W0612: Unused variable 'p' (unused-variable)
inspect.py:350:19: C0103: Argument name "fn" doesn't conform to snake_case naming style (invalid-name)
inspect.py:358:0: R0914: Too many local variables (19/15) (too-many-locals)
inspect.py:375:12: C0103: Variable name "rv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:382:8: C0103: Variable name "rv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:358:0: R0912: Too many branches (17/12) (too-many-branches)
inspect.py:449:21: W0612: Unused variable 'value' (unused-variable)
inspect.py:466:0: R0914: Too many local variables (27/15) (too-many-locals)
inspect.py:475:8: W0621: Redefining name 'graphviz' from outer scope (line 21) (redefined-outer-name)
inspect.py:475:8: C0415: Import outside toplevel (graphviz) (import-outside-toplevel)
inspect.py:476:4: C0103: Variable name "e" doesn't conform to snake_case naming style (invalid-name)
inspect.py:507:12: C0103: Variable name "rv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:542:12: C0103: Variable name "rv" doesn't conform to snake_case naming style (invalid-name)
inspect.py:466:0: R0912: Too many branches (16/12) (too-many-branches)
inspect.py:525:16: W0612: Unused variable 'plate2' (unused-variable)
inspect.py:556:0: R0913: Too many arguments (6/5) (too-many-arguments)

-----------------------------------
Your code has been rated at 8.00/10

It does not point anything at lines 607-610, so I assumed it was ok ...

Copy link
Member

Choose a reason for hiding this comment

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

I think you can go to the cloned numpyro folder and run make format. If you are using windows, probably running black *.py pyro and isort . will work. You need to install both black and isort to run those commands (pip install black isort)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info! I am in Linux, so I simply pip installed black isort as you mentioned and then, I have just run black over the entire pyro repository as black pyro/*.py. It announced some changes and that should be it? I also run isort pyro/*.py just in case
The command make format does not work (perhaps I need to install some additional stuff?), but the black and isort worked.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the line is too long, you can do

filepath = os.path.splitext(filename)[0]
graph.render(filepath, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey , dok, thanks for the patience, done!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, CI passed. It seems that you are using a black version different from CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, I am using:


black, 23.1.0 (compiled: yes)
Python (CPython) 3.8.13

@fehiepsi fehiepsi changed the title Issue #3184 Preserve path to the file when rendering model Mar 3, 2023
@fehiepsi fehiepsi merged commit 04fc486 into pyro-ppl:dev Mar 3, 2023
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