Skip to content

Add an option to create a filegraph in the dag rule#1574

Merged
fneum merged 11 commits intoPyPSA:masterfrom
tgilon:add_filegraph_rule
Mar 18, 2025
Merged

Add an option to create a filegraph in the dag rule#1574
fneum merged 11 commits intoPyPSA:masterfrom
tgilon:add_filegraph_rule

Conversation

@tgilon
Copy link
Copy Markdown
Collaborator

@tgilon tgilon commented Mar 12, 2025

Closes # (if applicable).

Changes proposed in this Pull Request

Checklist

  • I tested my contribution locally and it works as intended.
  • Code and workflow changes are sufficiently documented.
  • Changed dependencies are added to envs/environment.yaml.
  • Changes in configuration options are added in config/config.default.yaml.
  • Changes in configuration options are documented in doc/configtables/*.csv.
  • Sources of newly added data are documented in doc/data_sources.rst.
  • A release note doc/release_notes.rst is added.

@tgilon tgilon marked this pull request as ready for review March 12, 2025 16:18
fneum
fneum requested changes Mar 14, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@fneum fneum left a comment

Choose a reason for hiding this comment

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

Nice utility! But avoid definition of dag_type outside rule definition.

It seems the current approach mixes two approaches (wildcards and parameters). With the current setup you could actually use

snakemake --{dag_type} all | sed -n "/digraph/,\$p" > {output.dot}

to run snakemake dag_filegraph.pdf or snakemake dag_rulegraph.pdf and forget about the params or config setting.

I would also be fine to just create both rulegraph and filegraph on run snakemake dag.

Snakefile Outdated
Comment on lines +114 to +116
dag_type = config.get("dag_type", "rulegraph")


Copy link
Copy Markdown
Member

@fneum fneum Mar 14, 2025

Choose a reason for hiding this comment

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

Suggested change
dag_type = config.get("dag_type", "rulegraph")

Snakefile Outdated
conda:
"envs/environment.yaml"
params:
dag_type=dag_type,
Copy link
Copy Markdown
Member

@fneum fneum Mar 14, 2025

Choose a reason for hiding this comment

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

Suggested change
dag_type=dag_type,
dag_type=config.get("dag_type", "rulegraph"),

@tgilon
Copy link
Copy Markdown
Collaborator Author

tgilon commented Mar 14, 2025

Thanks @fneum for the review. Using only wildcards implies that we cannot have a default behaviour. That's why I suggest using config and a variable. I think the dag rule needs to be an easy to use rule as the dag is key to understanding the workflow (especially in the beginning).

# Not possible
snakemake dag

# Possible
snakemake resources/dag_rulegraph.pdf
snakemake resources/dag_filegraph.pdf

Also, as the filegraph may not be useful for everyone, I thought it would be a good idea to make it optional.

Would there be an implementation with wildcards and a default value?

@fneum
Copy link
Copy Markdown
Member

fneum commented Mar 17, 2025

Another option would be two separate rules:

snakemake rulegraph
snakemake filegraph

@fneum fneum merged commit a5b1489 into PyPSA:master Mar 18, 2025
2 checks passed
@tgilon tgilon deleted the add_filegraph_rule branch May 19, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fneum fneum fneum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tgilon @fneum