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

Create datasetId also for simulation dataframe (resolves #388) #408

Merged
merged 12 commits into from
Mar 26, 2020

Conversation

MerktSimon
Copy link
Member

Function check_ex_exp_columns is changed to also check simulation columns and create datasetId column in case it is missing

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #408 into develop will increase coverage by 0.03%.
The diff coverage is 73.91%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #408      +/-   ##
===========================================
+ Coverage    77.80%   77.84%   +0.03%     
===========================================
  Files           22       22              
  Lines         2118     2126       +8     
  Branches       494      498       +4     
===========================================
+ Hits          1648     1655       +7     
+ Misses         345      344       -1     
- Partials       125      127       +2     
Impacted Files Coverage Δ
petab/visualize/plot_data_and_simulation.py 74.24% <60.00%> (-1.52%) ⬇️
petab/visualize/helper_functions.py 88.15% <77.77%> (+0.21%) ⬆️
petab/visualize/plotting_config.py 50.57% <100.00%> (+2.38%) ⬆️

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 e911e32...ee6631e. Read the comment docs.

@elbaraim
Copy link
Collaborator

@MerktSimon but is not datasetId an optional field? why would you create it?

petab/visualize/helper_functions.py Outdated Show resolved Hide resolved
petab/visualize/helper_functions.py Outdated Show resolved Hide resolved
petab/visualize/plot_data_and_simulation.py Outdated Show resolved Hide resolved
@MerktSimon
Copy link
Member Author

@elbaraim it is creating the datasetId column only for the tables loaded as dataframe in order to continue with visualization. the local files stay untouched

@elbaraim
Copy link
Collaborator

@MerktSimon would be possible to silent this?

{'plot1': <matplotlib.axes._subplots.AxesSubplot at 0x7ff792722110>,
 'plot2': <matplotlib.axes._subplots.AxesSubplot at 0x7ff7926a0b50>,
 'plot3': <matplotlib.axes._subplots.AxesSubplot at 0x7ff79264a390>}

I think by adding plt.show() would work.

@MerktSimon
Copy link
Member Author

MerktSimon commented Mar 26, 2020

@elbaraim this is actually a feature not a bug.
plot_data_and_simulation() returns a dictionary of axes objects so that users can chose what to do with them.
You can silent this by e.g. ax = plot_data_and_simulation(...

@elbaraim
Copy link
Collaborator

absolutely! 😅

@MerktSimon MerktSimon merged commit d9260f0 into develop Mar 26, 2020
@MerktSimon MerktSimon deleted the fix388 branch March 26, 2020 11:37
raise ValueError(f'Visualization requires field {DATASET_ID} to be '
f'present in simulation table.')
if sim_data is not None:
sim_data[DATASET_ID] = exp_data[DATASET_ID]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this line is just asking for trouble. It might work for that one specific use case but is generally not a good solution.

  • you don't want to overwrite user provided value
  • I don't think we can generally assume that sim_data and exp_data are equally ordered.

Also I generally recommend that if you fix something like this, add one or two test cases to make sure it is not inadvertently broken in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • if sim_data is not None and DATASET_ID not in sim_data: Hence this only applies if there is no user provided datasetId. In these cases exp_data gets a default datasetId (something like 'obsverbale_condition').
  • I thought in cases without datasetId it would be reasonable to assume same order for sim_data and exp_data. But of course the procedure for creating exp_data's datasetId could be easily applied to sim_data, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

test wise this could be covered by enhancing each of the plot_by_observable/condition/datasetId tests with a second function call also plotting with simulated data and not only measurement

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • if sim_data is not None and DATASET_ID not in sim_data: Hence this only applies if there is no user provided datasetId. In these cases exp_data gets a default datasetId (something like 'obsverbale_condition').

I can only see if sim_data is not None there. As a proposal for a fix this sounds right.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah you are right. had to many intermediate versions.

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.

4 participants