-
Notifications
You must be signed in to change notification settings - Fork 37
Minor cleanup in datatypes #438
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #438 +/- ##
==========================================
+ Coverage 77.38% 78.13% +0.74%
==========================================
Files 327 329 +2
Lines 26085 26146 +61
==========================================
+ Hits 20187 20430 +243
+ Misses 5898 5716 -182 ☔ View full report in Codecov by Sentry. |
|
Love the idea of what you are doing ... in practice, data-types can be made way much simpler indeed, I started to do something similar some time ago, a very first mix of the MultiComponent Datatype, and the inclusion of the communicator as class attribute : https://github.com/Parallel-in-Time/pySDC/tree/master/pySDC/playgrounds/datatypes Did you look at it already ? There may be some stuff that can be useful in there (in particular, no need to override the new or __array__finalize methods of ndarray ...) |
|
Ah, nice! I forgot that you were doing this. Indeed, you can remove And then we can replace |
|
Any progress here? |
|
Personally, I vote to merge this and wait for a big cleanup. Nobody has time to change the entire interface of the datatypes now. This would be a small increment towards what @tlunet wants. |
| """ | ||
|
|
||
| def __new__(cls, init, val=0.0, offset=0, buffer=None, strides=None, order=None): | ||
| comm = None |
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.
Why not _comm here?
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.
I removed the getter function. I honestly don't see the point. But I can add it back in if you want.
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.
I "get" it. Let's keep it explicit, then.
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.
We can still have private "explicit" attributes denoted by the leading _, but no getter and setter (which are indeed superfluous)
While preparing for the NeuralPinT kickoff meeting, I thought we can trim the datatypes a lot by making the communicator a class attribute and do a nice
Tensordatatype in this way.Keep in mind that class attributes are shared between all instances of a class, but only across one task. That means we can assign different communicators to the class on different MPI processes and get essentially the same behaviour as before. I added a test to make sure this is actually the case.
Indeed, this allows to remove a lot of stuff from the datatypes.
Sadly, though, I noticed the problem with
Tensoris the frequent cloning of tensors, which I was not able to remove right now. So I didn't solve the problem I set out to solve, but I still consider this better than before.