Skip to content

Conversation

@cmcrook5
Copy link

@cmcrook5 cmcrook5 commented Jun 2, 2025

Added LvArray wrappers for ceil, floor, and power math functions that were previously missing from the develop branch. Also include polarDecomposition from Stefan's previous work.

@cmcrook5 cmcrook5 changed the title Feature/crook5/math Adding wrappers for device math functions and polar decomposition Jun 3, 2025
@cmcrook5 cmcrook5 requested review from rrsettgast and wrtobin June 3, 2025 15:44
@cmcrook5 cmcrook5 force-pushed the feature/crook5/math branch from c52307f to c3fcf1f Compare June 3, 2025 16:03
@cmcrook5 cmcrook5 self-assigned this Jun 17, 2025
Copy link
Collaborator

@corbett5 corbett5 left a comment

Choose a reason for hiding this comment

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

Marginal comments aside it looks good to me. I'd just add some tests for the math functions you added too.

The tensorOps stuff is pretty convoluted, but it looks you figured it out. Hope it wasn't too much trouble.

/**
* @brief Move assignment operator..
* @param src the SparsityPatternView to be moved from.
* @param src the ArrayOfArraysView to be moved from.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

if( size > 0 )
{
LVARRAY_ERROR_IF_NE_MSG( space, MemorySpace::host, "Calling reallocate with a non-zero current size is not yet supporeted for the GPU." );
LVARRAY_ERROR_IF_NE_MSG( space, MemorySpace::host, "Calling reallocate with a non-zero current size is not yet supported for the GPU." );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice


#if defined( LVARRAY_USE_DEVICE )

/// @copydoc ceil( T )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless you added tests for __half I'd just get rid of them.

}
};

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little unsure if this should go here or in GEOS itself.

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.

7 participants