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

ArrayView::size() should return ArrayView::size(0) #253

Open
klevzoff opened this issue Mar 24, 2022 · 1 comment
Open

ArrayView::size() should return ArrayView::size(0) #253

klevzoff opened this issue Mar 24, 2022 · 1 comment

Comments

@klevzoff
Copy link
Contributor

klevzoff commented Mar 24, 2022

Currently ArrayView::size() return the total number of elements across all dimensions. This creates a few inconveniences:

  • When dealing with various containers generically (e.g. Array<T,2> and ArrayOfArrays that may represent connectivity maps between different kinds of mesh objects), one cannot uniformly extract the primary dimension size with map.size(). We have to provide workarounds like various overloaded helper functions, whereas an element can be accessed uniformly with map(i,j).
  • An ArrayView can be viewed as a range of lower-dimensional slices, however size() does not return the size of that range (except in 1D case), and thus semantically it does not satisfy sized_range concept, so it may not be used with many of the C++ ranges algorithms (there are other problems as well, since we don't provide begin() and end() iterators for NDIM > 1, but I believe that can be fixed too).
  • On the other hand, interpreting a mutidimensional array as a range of all of its individual elements across all dimensions seems pointless, since it loses important semantics of dimensionality. I've not been able to find such use cases (which doesn't mean they don't exist, but I'd be curious to know).

I think it would be more consistent behavior if size() returned the same value as size(0) (or size(m_singleParameterResizeIndex), but in my opinion that member is useless and should just be removed). For use cases where the total number of elements is needed (e.g. allocating some buffer for serialization), a new member function numElements() could be introduced.

Since this is a subtle breaking change, it should be done with care. It's difficult to search for uses of .size() on arrays only in codebases, since it's called many times on all kinds of objects (vector, map, ArrayOfArrays, Array<T,1> for which there is no change, etc., and IDEs show too many false positives). So an agreement from all interested parties would be required.

@corbett5
Copy link
Collaborator

When dealing with various containers generically (e.g. Array<T,2> and ArrayOfArrays that may represent connectivity maps between different kinds of mesh objects), one cannot uniformly extract the primary dimension size with map.size(). We have to provide workarounds like various overloaded helper functions, whereas an element can be accessed uniformly with map(i,j).

I remember seeing these in GEOSX and I agree they're pretty ugly.

we don't provide begin() and end() iterators for NDIM > 1, but I believe that can be fixed too

There is actually a begin and end for anyArrayView, but the implementation forArraySlice will error out if it isn't contiguous. It would be nice to have an iterator that iterated over an arbitrary slice in logical order, but it's not super easy. Even just doing it in memory order would involve carrying around extra info.

On the other hand, interpreting a mutidimensional array as a range of all of its individual elements across all dimensions seems pointless, since it loses important semantics of dimensionality. I've not been able to find such use cases (which doesn't mean they don't exist, but I'd be curious to know)

There are plenty of valid use cases, such if you want to zero out an array or add a scalar or search... Plus it's more efficient to iterate over in this manner. But I agree most of the time you'll need/want all the indices.

but in my opinion that member (m_singleParameterResizeIndex) is useless and should just be removed

100% on board with this, I'm quite sure it's not used anywhere in GEOSX or anywhere else. Plus it would clean up the resizing logic.

Since this is a subtle breaking change, it should be done with care.

I'm on board with this change. I think a decent way to go about doing it would be to as you suggest make the no argument Array::size() return the size of the first dimension, but as a first step rename it to Array::fooBarXYZ(). Then in GEOSX you'll hopefully get a bunch of not so terrible compiler errors. This approach wouldn't work for things that are written to work on Array and std::vector, but those are few and far between. Finally you could do a search and replace to rename fooBarXYZ back to size.

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

No branches or pull requests

2 participants