Replaced CollectionBase with EasyList #304 - #305
Open
rozyczko wants to merge 2 commits into
Open
Conversation
rozyczko
added
[scope] maintenance
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## sampler-engine-structure-280 #305 +/- ##
================================================================
- Coverage 83.82% 83.66% -0.17%
================================================================
Files 68 68
Lines 5348 5271 -77
================================================================
- Hits 4483 4410 -73
+ Misses 865 861 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
damskii9992
approved these changes
Sep 9, 2026
damskii9992
left a comment
damskii9992
left a comment
Contributor
There was a problem hiding this comment.
Small comments only, otherwise it looks good to me.
Comment on lines
28
to
29
| fit_objects: list | None = None, | ||
| fit_functions: list[Callable] | None = None, |
Contributor
There was a problem hiding this comment.
Should this be typehinted as sequence, since we support any sequence?
Member
Author
There was a problem hiding this comment.
Yes. And more. We have to properly initialize fit_objects and fit_functions, otherwise the code will crash on the subsequent indexing... oops. Fixed.
Comment on lines
+214
to
+216
| For a ``MultiFitter`` this is not one of the supplied fit | ||
| objects but a read-only, indexable and iterable aggregate. | ||
|
|
Contributor
There was a problem hiding this comment.
Why a description for the MultiFitter in the Fitter?
Comment on lines
+233
to
+244
| def test_no_collection_base_deprecation_warning(self, caplog): | ||
| """Building a MultiFitter must not warn about CollectionBase. | ||
|
|
||
| The assertion is on message content rather than on the logger, | ||
| because other deprecated classes warn on the very same logger. | ||
| """ | ||
| fit_objects = [Line(1.0, 0.5), Line(2.0, 1.5)] | ||
|
|
||
| with caplog.at_level(logging.WARNING, logger='easyscience'): | ||
| MultiFitter(fit_objects, fit_objects) | ||
|
|
||
| assert not [r for r in caplog.records if 'CollectionBase is deprecated' in r.message] |
Comment on lines
+251
to
+264
| def test_fit_objects_are_not_retyped(self): | ||
| """The container must not reclassify the caller's fit objects. | ||
|
|
||
| The old CollectionBase dummy re-typed every fit object as | ||
| 'created_internal', hiding the caller's own objects from the | ||
| map's 'created' set. | ||
| """ | ||
| fit_objects = [Line(1.0, 0.5), Line(2.0, 1.5)] | ||
| types_before = [global_object.map.find_type(obj) for obj in fit_objects] | ||
|
|
||
| MultiFitter(fit_objects, fit_objects) | ||
|
|
||
| assert [global_object.map.find_type(obj) for obj in fit_objects] == types_before | ||
| assert all('created_internal' not in types for types in types_before) |
Contributor
There was a problem hiding this comment.
This is testing the global_object map? The one we want to get rid of? I'd remove this test.
Comment on lines
+318
to
+323
| def test_rejects_legacy_obj_base_fit_objects(self): | ||
| """Support for the deprecated ObjBase hierarchy was dropped.""" | ||
| legacy = [LegacyLine(1.0, 0.5), LegacyLine(2.0, 1.5)] | ||
|
|
||
| with pytest.raises(TypeError, match='Items must be one of'): | ||
| MultiFitter(legacy, legacy) |
Contributor
There was a problem hiding this comment.
Another test for deprecated content.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 pull request replaces the deprecated
CollectionBasecontainer with the newEasyListfor aggregating fit objects inMultiFitter.