-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Generators #866
Feature: Generators #866
Conversation
Benchmark is done. Checkout the benchmark result page. Benchmark diff v0.5.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)
|
Codecov Report
@@ Coverage Diff @@
## main #866 +/- ##
=======================================
- Coverage 85.4% 85.3% -0.1%
=======================================
Files 87 90 +3
Lines 4872 4857 -15
Branches 920 912 -8
=======================================
- Hits 4163 4147 -16
- Misses 558 561 +3
+ Partials 151 149 -2
Continue to review full report at Codecov.
|
9d8debf
to
f6aca9a
Compare
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
2159970
to
f0f7715
Compare
37bf142
to
9123bba
Compare
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
db3ed0b
to
733e4c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicky things
Please move
glotaran/testing/parallel_spectral_decay.py
->glotaran/testing/simulated_data/parallel_spectral_decay.py
glotaran/testing/sequential_spectral_decay.py
->glotaran/testing/simulated_data/sequential_spectral_decay.py
That way we can add a file glotaran/testing/simulated_data/common.py
and share common values (e.g. SIMULATION_COORDINATES
, PARAMETER_YML
), which will reduce code duplication.
As for disabling the quick-start
integration test (ca82e91), we should revert that commit and properly deprecate glotaran.examples
that is what glotaran.deprecation.modules
is for.
glotaran/project/generators/test/test_genenerate_decay_model.py
Outdated
Show resolved
Hide resolved
@joernweissenborn Don't forget to add the changes to the changelog. (@jsnel we still need to add the changes from #860) |
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>
for more information, see https://pre-commit.ci
7fb9ae5
to
05c1b5b
Compare
glotaran.testing.sequential_spectral_decay -> glotaran.testing.simulated_data.sequential_spectral_decay
2bba71d
to
af866cd
Compare
I made the generators keyword only and added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my side it is fine now 🚀
This allow showinging the correct new usage while importing the functionality from somewhere else.
8f9a656
to
19439ef
Compare
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.10%.
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! |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that glotaran.examples
is properly deprecated and all integration tests run again it should be good to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just some minor cosmetic changes requested (not all of them captured by the comments), but I will pick that up myself and then give it the green light.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be merged in now! 🎉
Note to @glotaran/maintainers: we should revisit #815 after merging in this PR.
Update, last messages was written under the assumption I merged in some changes, which I didn't. Migrated those changes to a separate follow up PR: |
Improve spelling, fix typos. Specific changes: - Use parameters instead of parameter where it makes sense. - Avoid using wildcards when importing from module
This PR salvages the generators from the project PR.
This is kind of in conflict with the work of @jsnel but we should use this PR to marry everything.
My approach of generators is more "dump", as the generating functions should return model dicts instead of models, I'am not sure why, but I think this will be useful.
Also this generators no't have any state, they are boilerplate replacement and I think all what is needed.
The usage will become more clear in a follow up PR where I add projects. This will also enable all the features the current Generator achieves with state fullness.
More details to follow
Change summary
glotaran.project.generators
packageChecklist
Closes issues
closes #XXXX