Skip to content
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

Ensure that Parameters and Mu instances are always ordered alphabetically #998

merged 4 commits into from Jul 7, 2020


Copy link

@sdrave sdrave commented Jul 6, 2020

In the past, there have been several issues with parameters not being ordered, e.g. #977. With this PR all Parameters and Mu instances will always be ordered alphabetically.

The implementation relies on Python >= 3.7 and CPython >= 3.6 dicts being insertion ordered.

Obviously this may have some impact on performance. (Creating a Frozendict(b=3, a=2) takes 1.5 microseconds on my machine whereas creating a SortedFrozenDict(b=3, a=2) takes 2.5 microseconds.) But I would assume that this is negligible in most cases, and avoiding subtle bugs due to invalid implicit assumptions seems more important.

@sdrave sdrave added the pr:change label Jul 6, 2020
@sdrave sdrave added this to the 2020.1 milestone Jul 6, 2020
@sdrave sdrave requested a review from renefritze Jul 6, 2020
Copy link
Member Author

@sdrave sdrave commented Jul 6, 2020

@TiKeil, please take a look ..

Copy link

@renefritze renefritze left a comment

Otherwise LGTM.

src/pymor/ Outdated Show resolved Hide resolved
Copy link

@TiKeil TiKeil commented Jul 6, 2020

Looks good. Thanks

@sdrave sdrave requested a review from renefritze Jul 6, 2020
Copy link

@codecov codecov bot commented Jul 6, 2020

Codecov Report

Merging #998 into master will decrease coverage by 0.00%.
The diff coverage is 89.47%.

Impacted Files Coverage Δ
src/pymor/tools/ 88.88% <87.50%> (-0.59%) ⬇️
src/pymor/parameters/ 79.65% <88.88%> (ø)
src/pymor/core/ 85.63% <100.00%> (+0.16%) ⬆️
src/pymor/vectorarrays/ 84.31% <0.00%> (-0.25%) ⬇️
src/pymor/vectorarrays/ 84.15% <0.00%> (-0.19%) ⬇️

@sdrave sdrave merged commit 0232879 into master Jul 7, 2020
10 checks passed
@sdrave sdrave deleted the sorted_params branch Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants