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

Updated and fixed the MapDimShuffle tranformation. #1531

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

philip-paul-mueller
Copy link
Collaborator

The reordering of the dimension was quite strange, it is now much more cleaner. Furthermore, the tile_sizes parameter of the map range was ignored, i.e. not changed. Furthermore, the parameters are now correclty copied. Before it was just assigned, thus if later the parameter list of the map was again changed this effect would propagate to all maps that where treated.

The reordering of the dimension was quite strange, it is now much more cleaner.
Furthermore, the `tile_sizes` parameter of the map range was ignored, i.e. not changed.
Furthermore, the parameters are now correclty copied.
Before it was just assigned, thus if later the parameter list of the map was again changed this effect would propagate to _all_ maps that where treated.
Copy link
Contributor

@lukastruemper lukastruemper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but the branches in L32 and L36 should be covered by tests then.

The `None` case is not tested, because such a transformation can not be created.
I have no idea why it was there, but it was there before.
@philip-paul-mueller
Copy link
Collaborator Author

@lukastruemper
I added one of the tests you asked, however, the None case can not be tested, because such a transformation can not be created.
The test was there before (I just moved it from the apply() to the can_be_applied() function) so I keep it.

Copy link
Contributor

@lukastruemper lukastruemper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can be merged once status checks pass and PR is up to date

@BenWeber42 BenWeber42 added this pull request to the merge queue Feb 27, 2024
Merged via the queue into spcl:master with commit b1a7f8a Feb 27, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants