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

misc: Fix EnrichedTuple rebuilding #2418

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Conversation

enwask
Copy link
Contributor

@enwask enwask commented Jul 19, 2024

Fixes EnrichedTuple (and its hierarchy) rebuilding to an empty instance.

@enwask enwask marked this pull request as ready for review July 19, 2024 15:18
@enwask enwask marked this pull request as draft July 19, 2024 15:29
@enwask enwask marked this pull request as ready for review July 19, 2024 16:28
devito/tools/data_structures.py Outdated Show resolved Hide resolved
devito/tools/data_structures.py Outdated Show resolved Hide resolved
@enwask enwask changed the title misc: Fix EnrichedTuple rebuilding misc: Fix EnrichedTuple + DimensionTuple rebuilding Jul 22, 2024
devito/types/utils.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.99%. Comparing base (4ef520b) to head (591c671).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2418   +/-   ##
=======================================
  Coverage   86.99%   86.99%           
=======================================
  Files         236      236           
  Lines       44821    44837   +16     
  Branches     8360     8361    +1     
=======================================
+ Hits        38991    39007   +16     
  Misses       5101     5101           
  Partials      729      729           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

devito/types/utils.py Outdated Show resolved Hide resolved
@FabioLuporini
Copy link
Contributor

could you also rebase the commit history as a single commit?

let me know if you need help with rebasing

@enwask
Copy link
Contributor Author

enwask commented Jul 23, 2024

if really needed, it would be a hack, which we cannot merge

@FabioLuporini I mean, is it? Arguably what's more of a hack is overriding instance __dict__ by EnrichedTuple.__new__, and adding arbitrary attributes when constructing it and its children (essentially creating anonymous classes).

I saw that left and right were the two attributes used consistently with DimensionTuple (namely a few times in AbstractFunction, once in DiscreteFunction._size_outhalo) so I thought those attributes should explicitly be a part of the class. I would argue that having these attributes semi-consistently exist on an instance, but not always, is objectively bad design.

I think ideally the hierarchy would be refactored such that those instance hacks aren't used anywhere and the attributes are explicit; that's why I at least thought the change should be made for DimensionTuple

@FabioLuporini
Copy link
Contributor

if really needed, it would be a hack, which we cannot merge

@FabioLuporini I mean, is it? Arguably what's more of a hack is overriding instance __dict__ by EnrichedTuple.__new__, and adding arbitrary attributes when constructing it and its children (essentially creating anonymous classes).

I think it's perfectly legal to alter __dict__ within __new__ for classes that have a dynamic number of kwargs.

That being said, I think I agree with the rest you've said. We should maybe just wrap a dict and pull from it each time we enter __getattr__. Sounds neater

@enwask
Copy link
Contributor Author

enwask commented Jul 23, 2024

Fair enough. So for now should I leave the explicit left/right rkwargs for DimensionTuple or get rid of them? Probably should have done this before but I can implement an override for EnrichedTuple._rebuild to reapply any attributes from __dict__ instead

@enwask enwask changed the title misc: Fix EnrichedTuple + DimensionTuple rebuilding misc: Fix EnrichedTuple rebuilding Jul 24, 2024
Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

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

for me this is now GTG, thanks. @mloubout ?

@mloubout
Copy link
Contributor

GTG, need rebase

@enwask
Copy link
Contributor Author

enwask commented Jul 26, 2024

Made some changes requested on Slack; refactored EnrichedTuple._getters -> .getters for cleaner construction, as well as overriding _rebuild so arbitrary attributes aren't discarded (previously rebuilding e.g. DimensionTuple(..., left=whatever) dropped the additional attributes).

Also added test_enrichedtuple_rebuild.

misc: refactor EnrichedTuple, fix DimensionTuple rebuilding

misc: clean up EnrichedTuple/DimensionTuple

misc: remove DimensionTuple rkwargs

misc: EnrichedTuple._getters -> getters

test: add EnrichedTuple._rebuild test
@mloubout mloubout merged commit 1a83ae6 into devitocodes:master Jul 26, 2024
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants