🩹 Fix yaml result saving with relative paths #1199

s-weigand · 2022-12-18T16:56:50Z

This fixes a bug when saving a result as yaml using a relative path, which added wrong file paths and broke loading the result.
We didn't discover this bug since all the examples use absolute paths.

To reproduce the issue use the following code snipped (e.g. in a notebook)

from glotaran.optimization.optimize import optimize
from glotaran.testing.simulated_data.parallel_spectral_decay import SCHEME

result = optimize(SCHEME)

result.save("results/result.yaml")

This generates the following files:

results
├── dataset_1.nc
├── initial_parameters.csv
├── model.yml
├── optimization_history.csv
├── optimized_parameters.csv
├── parameter_history.csv
├── result.md
├── result.yaml
└── scheme.yml

On main the paths pointing to other files used result.yaml and scheme.yml use a relative path to the current dir instead of relative to result.yaml.

result.yaml

number_of_function_evaluations: 3
success: true
termination_reason: '`ftol` termination condition is satisfied.'
glotaran_version: 0.7.0.dev0
free_parameter_labels:
- rates.species_1
- rates.species_2
- rates.species_3
- irf.center
- irf.width
scheme: results/scheme.yml
initial_parameters: results/initial_parameters.csv
optimized_parameters: results/optimized_parameters.csv
parameter_history: results/parameter_history.csv
optimization_history: results/optimization_history.csv
data:
  dataset_1: results/dataset_1.nc
chi_square: 15.08817310748813
degrees_of_freedom: 151195
number_of_data_points: 151200
number_of_jacobian_evaluations: 3
number_of_parameters: 5
optimality: 8.405348253083255e-06
reduced_chi_square: 9.979280470576494e-05
root_mean_square_error: 0.009989634863485499

scheme.yml

model: results/model.yml
parameters: results/initial_parameters.csv
data:
  dataset_1: dataset_1.nc
clp_link_tolerance: 0.0
clp_link_method: nearest
maximum_number_function_evaluations: null
add_svd: true
ftol: 1e-08
gtol: 1e-08
xtol: 1e-08
optimization_method: TrustRegionReflection
result_path: null

With this change, the paths are relative to the parent folder of result.yaml.

result.yaml

number_of_function_evaluations: 3
success: true
termination_reason: '`ftol` termination condition is satisfied.'
glotaran_version: 0.7.0.dev0
free_parameter_labels:
- rates.species_1
- rates.species_2
- rates.species_3
- irf.center
- irf.width
scheme: scheme.yml
initial_parameters: initial_parameters.csv
optimized_parameters: optimized_parameters.csv
parameter_history: parameter_history.csv
optimization_history: optimization_history.csv
data:
  dataset_1: dataset_1.nc
chi_square: 15.207098066318238
degrees_of_freedom: 151195
number_of_data_points: 151200
number_of_jacobian_evaluations: 3
number_of_parameters: 5
optimality: 3.0616701279760627e-06
reduced_chi_square: 0.00010057937144957332
root_mean_square_error: 0.010028926734679705

scheme.yml

model: model.yml
parameters: initial_parameters.csv
data:
  dataset_1: dataset_1.nc
clp_link_tolerance: 0.0
clp_link_method: nearest
maximum_number_function_evaluations: null
add_svd: true
ftol: 1e-08
gtol: 1e-08
xtol: 1e-08
optimization_method: TrustRegionReflection
result_path: null

Change summary

Checklist

✔️ Passing the tests (mandatory for all PR's)
🚧 Added changes to changelog (mandatory for all PR's)
🧪 Adds new tests for the feature (mandatory for ✨ feature and 🩹 bug fix PR's)

sourcery-ai · 2022-12-18T16:57:00Z

Sourcery Code Quality Report

❌ Merging this PR will decrease code quality in the affected files by 1.88%.

Quality metrics	Before	After	Change
Complexity	1.99 ⭐	2.18 ⭐	0.19 👎
Method Length	73.16 🙂	77.91 🙂	4.75 👎
Working memory	6.41 🙂	6.66 🙂	0.25 👎
Quality	76.07% ⭐	74.19% 🙂	-1.88% 👎

Other metrics	Before	After	Change
Lines	1020	1146	126

Changed files	Quality Before	Quality After	Quality Change
glotaran/builtin/io/yml/yml.py	72.28% 🙂	71.97% 🙂	-0.31% 👎
glotaran/builtin/io/yml/test/test_save_result.py	80.19% ⭐	64.63% 🙂	-15.56% 👎
glotaran/builtin/io/yml/test/test_save_scheme.py	82.67% ⭐	71.42% 🙂	-11.25% 👎
glotaran/utils/io.py	74.30% 🙂	74.79% 🙂	0.49% 👍
glotaran/utils/test/test_io.py	76.92% ⭐	77.28% ⭐	0.36% 👍

Here are some functions in these files that still need a tune-up:

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
glotaran/utils/io.py	_load_datasets	15 🙂	176 😞	12 😞	42.69% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/io/yml/yml.py	YmlProjectIo.save_model	15 🙂	117 🙂	13 😞	47.50% 😞	Extract out complex expressions
glotaran/utils/io.py	create_clp_guide_dataset	6 ⭐	162 😞	11 😞	53.86% 🙂	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/io/yml/test/test_save_result.py	test_save_result_yml	3 ⭐	188 😞	11 😞	54.46% 🙂	Try splitting into smaller methods. Extract out complex expressions
glotaran/utils/test/test_io.py	test_load_datasets_single_dataset	0 ⭐	242 ⛔	7 🙂	62.32% 🙂	Try splitting into smaller methods

Legend and Explanation

The emojis denote the absolute quality of the code:

⭐ excellent
🙂 good
😞 poor
⛔ very poor

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!

github-actions · 2022-12-18T16:57:02Z

👈 Launch a binder notebook on branch s-weigand/pyglotaran/fix-save-result

github-actions · 2022-12-18T17:00:18Z

Benchmark is done. Checkout the benchmark result page.
Benchmark differences below 5% might be due to CI noise.

Benchmark diff v0.6.0 vs. main

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [6c3c390e]       [b5698b88]
     <v0.6.0>                   
!      44.1±0.3ms           failed      n/a  BenchmarkOptimize.time_optimize(False, False, False)
!      46.7±0.7ms           failed      n/a  BenchmarkOptimize.time_optimize(False, False, True)
!      43.9±0.3ms           failed      n/a  BenchmarkOptimize.time_optimize(False, True, False)
!      46.8±0.3ms           failed      n/a  BenchmarkOptimize.time_optimize(False, True, True)
!      54.1±0.2ms           failed      n/a  BenchmarkOptimize.time_optimize(True, False, False)
!       63.1±10ms           failed      n/a  BenchmarkOptimize.time_optimize(True, False, True)
!      53.9±0.2ms           failed      n/a  BenchmarkOptimize.time_optimize(True, True, False)
!        60.7±2ms           failed      n/a  BenchmarkOptimize.time_optimize(True, True, True)
             203M             207M     1.02  IntegrationTwoDatasets.peakmem_optimize
-      1.91±0.07s       1.01±0.03s     0.53  IntegrationTwoDatasets.time_optimize

Benchmark diff main vs. PR

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [b5698b88]       [3d0dec55]
           failed           failed      n/a  BenchmarkOptimize.time_optimize(False, False, False)
           failed           failed      n/a  BenchmarkOptimize.time_optimize(False, False, True)
           failed           failed      n/a  BenchmarkOptimize.time_optimize(False, True, False)
           failed           failed      n/a  BenchmarkOptimize.time_optimize(False, True, True)
           failed           failed      n/a  BenchmarkOptimize.time_optimize(True, False, False)
           failed           failed      n/a  BenchmarkOptimize.time_optimize(True, False, True)
           failed           failed      n/a  BenchmarkOptimize.time_optimize(True, True, False)
           failed           failed      n/a  BenchmarkOptimize.time_optimize(True, True, True)
             207M             205M     0.99  IntegrationTwoDatasets.peakmem_optimize
       1.01±0.03s         993±20ms     0.98  IntegrationTwoDatasets.time_optimize

glotaran/utils/test/test_io.py

codecov · 2022-12-18T17:01:09Z

Codecov Report

Base: 87.7% // Head: 87.7% // Increases project coverage by +0.0% 🎉

Coverage data is based on head (3d0dec5) compared to base (b5698b8).
Patch coverage: 100.0% of modified lines in pull request are covered.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #1199   +/-   ##
=====================================
  Coverage   87.7%   87.7%           
=====================================
  Files        104     104           
  Lines       4954    4968   +14     
  Branches     823     824    +1     
=====================================
+ Hits        4345    4359   +14     
  Misses       492     492           
  Partials     117     117

Impacted Files	Coverage Δ
glotaran/builtin/io/yml/yml.py	`88.7% <100.0%> (+0.2%)`	⬆️
glotaran/utils/io.py	`94.6% <100.0%> (+0.6%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

sonarqubecloud · 2022-12-18T19:12:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jsnel

LGTM!

s-weigand added 3 commits December 18, 2022 17:06

✨ Added chdir_context context manager for testing with relative paths

ad96e6f

🧪 Added failing test to reproduce save_result issue with relative paths

e5f083b

🩹 Fix wrong paths in result.yml when using relative path

2f1643b

joernweissenborn previously approved these changes Dec 18, 2022

View reviewed changes

🚧📚 Added change to changelog

12c0ea0

github-advanced-security bot found potential problems Dec 18, 2022

View reviewed changes

glotaran/utils/test/test_io.py Show resolved Hide resolved

s-weigand dismissed joernweissenborn’s stale review via 12c0ea0 December 18, 2022 18:04

s-weigand added 2 commits December 18, 2022 19:50

🧪 Added test where scheme data input path is different than save path

3cb18c4

🩹 Fixed dataset path in saved scheme

3d0dec5

s-weigand force-pushed the fix-save-result branch from febe078 to 3d0dec5 Compare December 18, 2022 19:11

jsnel approved these changes Dec 18, 2022

View reviewed changes

s-weigand merged commit 46dad41 into glotaran:main Dec 18, 2022

s-weigand deleted the fix-save-result branch December 18, 2022 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🩹 Fix yaml result saving with relative paths #1199

🩹 Fix yaml result saving with relative paths #1199

s-weigand commented Dec 18, 2022 •

edited

Loading

sourcery-ai bot commented Dec 18, 2022 •

edited

Loading

github-actions bot commented Dec 18, 2022

github-actions bot commented Dec 18, 2022 •

edited

Loading

codecov bot commented Dec 18, 2022 •

edited

Loading

sonarqubecloud bot commented Dec 18, 2022 •

edited

Loading

jsnel left a comment

🩹 Fix yaml result saving with relative paths #1199

🩹 Fix yaml result saving with relative paths #1199

Conversation

s-weigand commented Dec 18, 2022 • edited Loading

Change summary

Checklist

sourcery-ai bot commented Dec 18, 2022 • edited Loading

Sourcery Code Quality Report

Legend and Explanation

github-actions bot commented Dec 18, 2022

github-actions bot commented Dec 18, 2022 • edited Loading

codecov bot commented Dec 18, 2022 • edited Loading

Codecov Report

sonarqubecloud bot commented Dec 18, 2022 • edited Loading

jsnel left a comment

Choose a reason for hiding this comment

s-weigand commented Dec 18, 2022 •

edited

Loading

sourcery-ai bot commented Dec 18, 2022 •

edited

Loading

github-actions bot commented Dec 18, 2022 •

edited

Loading

codecov bot commented Dec 18, 2022 •

edited

Loading

sonarqubecloud bot commented Dec 18, 2022 •

edited

Loading