Skip to content

Conversation

bowni
Copy link
Member

@bowni bowni commented Apr 1, 2021

Changes:

  • Results are now always saved
  • Two cases taken into account: whether a scenario is run as standalone or as part of an Experiment

Fixes #329, #331

@bowni bowni added bug Something isn't working enhancement New feature or request labels Apr 1, 2021
@bowni bowni self-assigned this Apr 1, 2021
@bowni bowni requested a review from Thomas-Galtier as a code owner April 1, 2021 14:43
Copy link
Contributor

@JustineBoulant JustineBoulant left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@RomainGoussault RomainGoussault left a comment

Choose a reason for hiding this comment

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

Could we add some test for this @bowni?
I was thinking of running an experiment and checking there is a result file that was created. (and same thing for running a scenario).

@bowni bowni added this to the Release v0.4 milestone Jul 13, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #337 (8da900d) into master (4fb247a) will decrease coverage by 0.06%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   80.82%   80.76%   -0.07%     
==========================================
  Files          15       15              
  Lines        3088     3099      +11     
==========================================
+ Hits         2496     2503       +7     
- Misses        592      596       +4     
Impacted Files Coverage Δ
mplc/scenario.py 80.06% <61.90%> (-2.44%) ⬇️
mplc/experiment.py 83.33% <83.33%> (+2.46%) ⬆️
mplc/constants.py 100.00% <100.00%> (ø)
mplc/multi_partner_learning/basic_mpl.py 85.71% <100.00%> (ø)
mplc/multi_partner_learning/utils.py 86.86% <0.00%> (+0.72%) ⬆️
mplc/models.py 79.38% <0.00%> (+1.03%) ⬆️

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 4fb247a...8da900d. Read the comment docs.

@RomainGoussault
Copy link
Member

Could we add some test for this @bowni?
I was thinking of running an experiment and checking there is a result file that was created. (and same thing for running a scenario).

@bowni I think it's still useful to add a test. What do you think?

Copy link
Member

@RomainGoussault RomainGoussault left a comment

Choose a reason for hiding this comment

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

LGTM !

@bowni bowni merged commit eb20b80 into master Sep 14, 2021
@bowni bowni deleted the clarify-saving-results branch September 14, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify in documentation the use of save_path in Scenario and Experiment
4 participants