Refactor Result Saving #841
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joernweissenborn
requested review from
jsnel,
s-weigand and
a team
as code owners
|
|
|
Benchmark is done. Checkout the benchmark result page. Benchmark diff v0.4.1 vs. mainParametrized benchmark signatures: BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)Benchmark diff main vs. PRParametrized benchmark signatures: BenchmarkOptimize.time_optimize(index_dependent, grouped, weight) |
s-weigand
reviewed
Sep 30, 2021
s-weigand
reviewed
Sep 30, 2021
s-weigand
force-pushed
the
refactor/result_saving
branch
from
086cec9 to
78a6b5c
Compare
Codecov Report
@@ Coverage Diff @@
## main #841 +/- ##
=======================================
- Coverage 84.9% 84.5% -0.4%
=======================================
Files 77 79 +2
Lines 4372 4522 +150
Branches 785 826 +41
=======================================
+ Hits 3712 3824 +112
- Misses 521 556 +35
- Partials 139 142 +3
Continue to review full report at Codecov.
|
jsnel
reviewed
Oct 3, 2021
Signed-off-by: Jörn Weißenborn <joern.weissenborn@gmail.com>
Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>
As a side effect it sets the file paths of saved files in the Result object.
s-weigand
requested changes
Oct 10, 2021
There was a problem hiding this comment.
Sorry for the long wait on my review.
In general, I don't like the cluttering of the data classes with the <attr>_file attributes.
So in a follow-up PR we should refactor this to a more elegant solution.
E.g.:
1.) Loaders overwrite an origin_file attribute ({"path": <file_path>, "format_name": <format_name>} ) on the objects (Model, ParameterGroup, ...) which we can set to a default value.
2.) We use a file_mapping attribute {"model": {"path": <file_path>, "format_name": <format_name>}, ....}
Since each object has one loader plugin we can make it a property of the object itself.
Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>
Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>
Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>
Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>
|
Kudos, SonarCloud Quality Gate passed!
|
…p_path instead of tmpdir
s-weigand
force-pushed
the
refactor/result_saving
branch
from
b9217b4 to
533598c
Compare
For some reason when running pre-commit on pre-commit-ci the exclude settings of interrogate are ignored. Compare: https://github.com/glotaran/pyglotaran/runs/3853222703 vs. https://results.pre-commit.ci/run/github/58401715/1633899505.QoeN82EHTouKusNC0fMSCg But since we run it in our github actions workflow it is save to deactivate it for pre-commit-ci.
s-weigand
force-pushed
the
refactor/result_saving
branch
from
26fb1a3 to
144b4d1
Compare
This was referenced Oct 12, 2021
|
Kudos, SonarCloud Quality Gate passed!
|
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.83%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
This was referenced Oct 12, 2021
jsnel
approved these changes
Oct 12, 2021
There was a problem hiding this comment.
Reviewed ok, taking into account some new minor issues (#855)
3 tasks
This was referenced Apr 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the way results are saved and enables loading and validating results.
Change summary
Checklist
Closes issues
closes #323
closes #317
closes #316
closes #767