dash icon indicating copy to clipboard operation
dash copied to clipboard

removed unnecessary inheritance

Open dhinf opened this issue 5 years ago • 11 comments

bug fix for issue #631

dhinf avatar Mar 05 '19 10:03 dhinf

Codecov Report

Merging #632 into development will decrease coverage by 0.13%. The diff coverage is n/a.

@@               Coverage Diff               @@
##           development     #632      +/-   ##
===============================================
- Coverage        84.95%   84.81%   -0.14%     
===============================================
  Files              335      335              
  Lines            24798    24798              
  Branches         11249    11262      +13     
===============================================
- Hits             21066    21033      -33     
  Misses            3729     3729              
- Partials             3       36      +33
Impacted Files Coverage Δ
dash/include/dash/halo/Halo.h 97.61% <ø> (ø) :arrow_up:
dash/include/dash/internal/Logging.h 68.18% <0%> (-31.82%) :arrow_down:
dash/test/TestBase.h 68.18% <0%> (-22.73%) :arrow_down:
dash/include/dash/Team.h 83.15% <0%> (-9.48%) :arrow_down:

codecov[bot] avatar Mar 05 '19 10:03 codecov[bot]

Could you test it with Intel 19? I could send this one-liner to the original report, but it may take some time to get feedback.

bertwesarg avatar Mar 05 '19 12:03 bertwesarg

RegionSpec is not really "a" Dimensional. The member RegionCoords is Dimensional. But RegionSpec only combines RegionCoords with some other properties.

dhinf avatar Mar 11 '19 08:03 dhinf

Well, the question whether "A is a B" (A : public B) means: "Can you pass an instance of A to every context that expects a B? In this example, let's assume:

SizeT volume(const Dimensional & d) { return std::accumulate( ... , std::multiplies); }
RegionSpec<...> rs(...);
auto region_volume = volume(rs);

I don't see why something like that should not be okay, so RegionSpec could act as Dimensional. Back then, I added some other types for Cartesian domains, perhaps something else is a better fit We'll see tomorrow.

fuchsto avatar Mar 11 '19 13:03 fuchsto

What is the way forward here? I don't remember this was discussed at the F2F but it keep biting...

devreal avatar Mar 28 '19 08:03 devreal

i still think that no inheritance should be used. I don't use the data structure of the base class or any methods.

dhinf avatar Mar 28 '19 09:03 dhinf

Can the RegionSpec be implemented in terms of Dimensional?

devreal avatar Mar 28 '19 09:03 devreal

Not really, because RegionSpec combines RegionCoords (Dimensional), the extent and some other useful things. In an older version i didn't used RegionCoords, but instead Dimensional to store the coordinates. Later i devided them. And at this point i missed to delete the inheritance.

dhinf avatar Mar 28 '19 09:03 dhinf

In an older version i didn't used RegionCoords, but instead Dimensional to store the coordinates

What's the reason to not use Dimensional to store the coordinates anymore?

devreal avatar Mar 28 '19 09:03 devreal

Because i needed RegionCoords separately for other components. But seriously, i don't understand the problem here. We have so much other, more urgent, stuff to do.

dhinf avatar Mar 28 '19 11:03 dhinf

@fuchsto I think we should merge this PR as is. Can you accept RegionSpec not being a Dimensional?

devreal avatar Apr 30 '19 20:04 devreal