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.

Sign up for GitHub

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

Jump to bottom

Visualization #405

Merged
merged 8 commits into from
Mar 24, 2020
Merged

Visualization #405

merged 8 commits into from
Mar 24, 2020

Conversation

plakrisenko
Copy link
Member

#194
documentation update
visualization without visu_spec fix

@codecov
Copy link

codecov bot commented Mar 24, 2020 •
edited
Loading

Codecov Report

Merging #405 into develop will decrease coverage by 0.07%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #405      +/-   ##
==========================================
- Coverage    77.88%   77.8%   -0.08%     
==========================================
  Files           22      22              
  Lines         2116    2118       +2     
  Branches       493     494       +1     
==========================================
  Hits          1648    1648              
- Misses         344     345       +1     
- Partials       124     125       +1
Impacted Files Coverage Δ
petab/visualize/plotting_config.py 48.19% <ø> (ø) ⬆️
petab/visualize/data_overview.py 100% <ø> (ø) ⬆️
petab/visualize/helper_functions.py 87.94% <0%> (-0.8%) ⬇️
petab/visualize/plot_data_and_simulation.py 75.75% <75%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e713f8b...cfc1d50. Read the comment docs.

@plakrisenko plakrisenko requested a review from MerktSimon March 24, 2020 10:29
yannikschaelte
yannikschaelte approved these changes Mar 24, 2020
View reviewed changes
Copy link
Member

@yannikschaelte yannikschaelte left a comment

Choose a reason for hiding this comment

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

looks good. one could simplify things by indicating no visualization file available by passing None (or nothing, using default values) instead of ''.

doc/documentation_data_format.md Outdated
@@ -447,14 +447,14 @@ order:

*(wrapped for readability)*

| ... | [xValues] | [xOffset] | [xLabel] | xScale |
| ... | [xValues] | [xOffset] | [xLabel] | [xScale] |
|---|---|---|---|---|
|... | [time(default)&#124;parameterOrStateId] | [NUMERIC] | [STRING] | lin&#124;log&#124;log10&#124;order |
Copy link
Member

Choose a reason for hiding this comment

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

please also add [] here to indicate it as optional

doc/documentation_data_format.md Outdated
|---|---|---|---|---|
|... | [time(default)&#124;parameterOrStateId] | [NUMERIC] | [STRING] | lin&#124;log&#124;log10&#124;order |
|...|...|...|...|

*(wrapped for readability)*

| ... | yValues | [yOffset] | [yLabel] | yScale | [legendEntry] |
| ... | yValues | [yOffset] | [yLabel] | [yScale] | [legendEntry] |
|---|---|---|---|---|---|
|... | observableId | [NUMERIC] | [STRING] | lin&#124;log&#124;log10 | [STRING] |
Copy link
Member

Choose a reason for hiding this comment

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

see above

petab/visualize/helper_functions.py Outdated
@@ -40,9 +40,14 @@ def import_from_files(
pd.DataFrame, pd.DataFrame]:
"""
Helper function for plotting data and simulations, which imports data
from PEtab files.
from PEtab files. If path to the visualization file is not provided, the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from PEtab files. If path to the visualization file is not provided, the
from PEtab files. If `visualization_file_path` is not provided, the

can one make visualization_file_path optional too? above, -> visualization_file_path: str = None, and move one down. and below, change the check for visualization_file_path != '' to visualization_file_path not in ['', None]

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe just if visualization_file_path instead of visualization_file_path != ''

petab/visualize/plot_data_and_simulation.py Outdated
if isinstance(vis_spec, str):
if vis_spec != '':
if vis_spec:
if isinstance(vis_spec, str):
Copy link
Member

Choose a reason for hiding this comment

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

not sure if that check is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, if vis_spec can be None, '' and str, then, I think, it's needed.
But maybe excluding '' is better

@plakrisenko plakrisenko merged commit 2fe507e into develop Mar 24, 2020
@plakrisenko plakrisenko deleted the pl_visualization branch March 26, 2020 15:37
@yannikschaelte yannikschaelte mentioned this pull request Mar 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yannikschaelte yannikschaelte yannikschaelte approved these changes

@MerktSimon MerktSimon Awaiting requested review from MerktSimon

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
@plakrisenko @yannikschaelte