Skip to content

Introduction of runnable base class to assist with introduction of fitting specs#253

Merged
jonc125 merged 23 commits intomasterfrom
134-fitting-result
Apr 30, 2020
Merged

Introduction of runnable base class to assist with introduction of fitting specs#253
jonc125 merged 23 commits intomasterfrom
134-fitting-result

Conversation

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

@sroderick-g5sro sroderick-g5sro commented Feb 27, 2020

Part of #134

@sroderick-g5sro
Copy link
Copy Markdown
Contributor Author

sroderick-g5sro commented Feb 27, 2020

Hi @jonc125. I am submitting this pull request for your review but I am aware of a number of issues:

  1. I am not sure that I have taken the best course through the migrations and there maybe a number of redundant steps.
  2. In one step renaming ExperimentVersion to Runnable meant that I had to change all references in the code to generate the automatic migration. In particular I had to remove code in experiments/admin.py I have readded that and reverted the global reference changes. These steps didn't "feel" right and there may have been a simpler approach.
  3. In migration 0028_auto_20200212_1701.py I have added the logic to migrate the experiment_version records. I haven't added a reverse step to remove these - which I am guessing is required? Also I probably need to rename the file to something other than auto.
  4. I have fixed the resulting unit test failures for example when fetching related experiments I have changed the code from experiment_version__experiment to experiment_version__experimentversion__experiment. I am assuming that I can do this double hop as the test pass.
  5. There is one test failure that I am still working on. Experiment>test_sends_mail_when_experiment_is_finished

In general it would be good to get your feedback on the approach I have taken in this change please

@sroderick-g5sro sroderick-g5sro changed the title Introduction of runnable entity Introduction of runnable base class to assist with introduction of fitting specs Feb 27, 2020
Comment thread weblab/experiments/apps.py
Comment thread weblab/experiments/forms.py Outdated
Comment thread weblab/experiments/migrations/0028_auto_20200212_1701.py Outdated
Copy link
Copy Markdown
Contributor

@jonc125 jonc125 left a comment

Choose a reason for hiding this comment

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

This was a complicated one to get right! I've worked through the migrations, and think I can see a pathway to getting it working. See what you think of my suggestions.

Comment thread weblab/experiments/apps.py Outdated
Comment thread weblab/experiments/models.py
Comment thread weblab/experiments/models.py
Comment thread weblab/experiments/models.py Outdated
Comment thread weblab/experiments/processing.py Outdated
Comment thread weblab/experiments/migrations/0027_experimentversion_experiment_key.py Outdated
Comment thread weblab/experiments/migrations/0028_auto_20200212_1701.py Outdated
Comment thread weblab/experiments/migrations/0031_remove_runnable_experiment.py Outdated
Comment thread weblab/experiments/apps.py
Comment thread weblab/experiments/forms.py Outdated
Comment thread weblab/config/settings/base.py Outdated
sroderick-g5sro and others added 8 commits February 28, 2020 17:36
These were created by removing much of the view logic etc (but not
committing) so I could get Django to make the migrations automatically,
with the exception of the one RunPython migration to move the experiment
keys between models. Experiment tests pass and I can now view the matrix
after migrating my local data. I'd recommend more manual testing that
things still work before merging though!
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 17, 2020

Codecov Report

Merging #253 into master will decrease coverage by <.1%.
The diff coverage is 92.8%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #253     +/-   ##
========================================
- Coverage    94.5%   94.5%   -0.1%     
========================================
  Files          65      65             
  Lines        2685    2689      +4     
  Branches      280     280             
========================================
+ Hits         2540    2543      +3     
- Misses        105     106      +1     
  Partials       40      40
Impacted Files Coverage Δ
weblab/experiments/views.py 98.3% <ø> (ø) ⬆️
weblab/conftest.py 99.3% <100%> (ø) ⬆️
weblab/experiments/apps.py 100% <100%> (ø) ⬆️
weblab/experiments/signals.py 100% <100%> (ø) ⬆️
weblab/experiments/emails.py 100% <100%> (ø) ⬆️
weblab/core/recipes.py 100% <100%> (ø) ⬆️
weblab/entities/views.py 93% <100%> (ø) ⬆️
weblab/experiments/models.py 98.1% <87.5%> (-1%) ⬇️
weblab/experiments/processing.py 92.5% <90%> (ø) ⬆️

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 013fe77...b10b447. Read the comment docs.

@jonc125 jonc125 merged commit 51994ee into master Apr 30, 2020
@jonc125 jonc125 deleted the 134-fitting-result branch April 30, 2020 10:23
MauriceHendrix pushed a commit that referenced this pull request Jul 22, 2021
Introduction of runnable base class to assist with introduction of fitting specs
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.

3 participants