♻️ Refactor: Model and Parameters #1135
Conversation
joernweissenborn
requested review from
jsnel,
s-weigand and
a team
as code owners
|
|
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.32%.
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 pull request introduces 2 alerts and fixes 1 when merging ab66705 into e1467ec - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 2 alerts and fixes 1 when merging 23e3082 into e1467ec - view on LGTM.com new alerts:
fixed alerts:
|
Codecov ReportBase: 87.2% // Head: 87.6% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1135 +/- ##
=======================================
+ Coverage 87.2% 87.6% +0.4%
=======================================
Files 103 103
Lines 5274 4899 -375
Branches 916 807 -109
=======================================
- Hits 4599 4292 -307
+ Misses 527 489 -38
+ Partials 148 118 -30
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. |
|
This pull request introduces 2 alerts and fixes 1 when merging 02f732a into e1467ec - view on LGTM.com new alerts:
fixed alerts:
|
s-weigand
changed the title
joernweissenborn
force-pushed
the
refactor/model_and_parameters
branch
from
02f732a to
af920c2
Compare
|
This pull request introduces 2 alerts and fixes 1 when merging af920c2 into 9d5119c - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 2 alerts and fixes 1 when merging 5bddb96 into 9d5119c - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 2 alerts and fixes 1 when merging 14968fe into 9d5119c - view on LGTM.com new alerts:
fixed alerts:
|
|
Benchmark is done. Checkout the benchmark result page. Benchmark diff v0.6.0 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) |
|
This pull request introduces 2 alerts and fixes 1 when merging 97503db into 9d5119c - view on LGTM.com new alerts:
fixed alerts:
|
|
@joernweissenborn I temporarily changed the CI to only run python 3.10 so you get feedback from the integration tests. |
|
This pull request introduces 2 alerts and fixes 1 when merging 030a713 into 9d5119c - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 2 alerts and fixes 1 when merging ef9a765 into 9d5119c - view on LGTM.com new alerts:
fixed alerts:
|
|
As is clear from the previous comments in this PR, the incoming changes effectively require raising the minimum supported Python version from 3.8 to 3.10, due to relying on the improvements to the typing system added in 3.10. After some internal deliberation we have concluded that the benefits this PR bring us (cleaner and more maintainable model definitions) outweigh the cost of dropping 3.8 and 3.9 support, even though these versions are still supported by the wider Python community. However, for posterity I want to record that we did carefully consider the impact of these changes and don't consider taking such a change lightly. Other than the aforementioned advantages the incoming code changes bring us, our reason for going ahead is two fold:
Hopefully this provides enough motivation and documentation for the incoming change in supported Python versions. Part of this will also be added to the README. |
|
@s-weigand the two commits to raise the minimum version of Python to 3.10 can be made more permanent (change from ❌ to 🚧 or 🚇 or ⬆️), and add a commit to update the changelog and readme to reflect this change. e.g.: ♻️ Refactored internal Model and Parameter code definition using modern Python 3.10 typing (#1135) |
|
@jsnel I will add a |
|
This pull request introduces 2 alerts and fixes 1 when merging a4b195a into 9d5119c - view on LGTM.com new alerts:
fixed alerts:
|
jsnel
force-pushed
the
refactor/model_and_parameters
branch
from
b0f646c to
90ac37a
Compare
|
This pull request introduces 2 alerts and fixes 1 when merging 90ac37a into c8d7722 - view on LGTM.com new alerts:
fixed alerts:
|
|
Issues with some checks not being executed:
So before we merge we'll fix the branch protection settings. |
|
This pull request introduces 2 alerts and fixes 1 when merging a23d15e into c8d7722 - view on LGTM.com new alerts:
fixed alerts:
|
The unitest benchmarks are still partially broken since we decided that they are not very informative since they mostly reflect setup time
|
This pull request introduces 2 alerts and fixes 1 when merging c5996bf into c8d7722 - view on LGTM.com new alerts:
fixed alerts:
|
There was a problem hiding this comment.
From my side this is fine to merge.
Thanks @joernweissenborn for making the code a lot more readable and maintainable. 🎉
Fixes redefined-builtin (W0622)
|
This pull request introduces 2 alerts and fixes 1 when merging f96340f into c8d7722 - view on LGTM.com new alerts:
fixed alerts:
|
Line added: ♻️ Complete refactor of model and parameter packages using attrs (glotaran#1135)
|
Kudos, SonarCloud Quality Gate passed!
|
|
This pull request introduces 2 alerts and fixes 1 when merging 3c2006e into c8d7722 - view on LGTM.com new alerts:
fixed alerts:
|
glotaran/builtin/megacomplexes/damped_oscillation/test/test_doas_model.py
Show resolved
Hide resolved
PR glotaran#1135 changed the colmn names from 'non-negative' -> 'non_negative' and 'standard-error' -> 'standard_error' which causes crashes in load_parameters when reading parameters files that were created before the merge (e.g. 'optimized_parameters.csv'). This change allows reading of those old files, while saving still uses the new version
PR glotaran#1135 changed the colmn names from 'non-negative' -> 'non_negative' and 'standard-error' -> 'standard_error' which causes crashes in load_parameters when reading parameters files that were created before the merge (e.g. 'optimized_parameters.csv'). This change allows reading of those old files, while saving still uses the new version
…1174) * 🩹 Fix reading of old pandas saved parameters files PR #1135 changed the column names from 'non-negative' -> 'non_negative' and 'standard-error' -> 'standard_error' which causes crashes in load_parameters when reading parameters files that were created before the merge (e.g. 'optimized_parameters.csv'). This change allows reading of those old files, while saving still uses the new version * 🚧📚 Added change to change log * 🧹 Moved column renaming resposibility to plugin implementation 👌 Allow alternate notation used in yaml for pandas plugins 👌 Make column names case insensitive for pandas plugins * 🧪 Added unittest for Parameters.from_dataframe column validation * 🚧📚 Updated changelog with changed PR title * 🧪Added parameters subset csv test Co-authored-by: Joris Snellenburg <[email protected]>
This PR completely reimplements the model and parameter package.
Instead of using our own homegrown metaprogramming solution from 4 years ago, this change utilizes the battle-proven
attrslibrary and modern python typing (3.10).This improves the code readability, static type checking and maintainability.
Change summary
attrsChecklist
Closes issues
closes #917