core: implement np.any() & np.all() #424

Open
lyken wants to merge 2 commits from numpy-anyall into master
Collaborator

Implements #279.

New demos have also been added to nac3standalone/

Implements https://git.m-labs.hk/M-Labs/nac3/issues/279. New demos have also been added to `nac3standalone/`
lyken requested review from derppening 2024-06-19 17:30:20 +08:00
sb10q reviewed 2024-06-19 17:50:17 +08:00
@ -2774,0 +2872,4 @@
// Immediately begin iterating through the ndarray
ctx.builder.build_unconditional_branch(step_bb).unwrap();
// ##### Inserting into `step_bb` #####
Owner

Nitpick but we don't use this comment style anywhere else.

Nitpick but we don't use this comment style anywhere else.
derppening marked this conversation as resolved
sb10q reviewed 2024-06-19 17:50:37 +08:00
@ -1760,0 +1763,4 @@
debug_assert_prim_is_allowed(prim, &[PrimDef::FunNpAny, PrimDef::FunNpAll]);
let param_ty = &[(self.ndarray_num_ty, "a")];
let ret_ty = self.primitives.bool;
// let var_map = &into_var_map([self.ndarray_dtype_tvar, self.ndarray_ndims_tvar]);
Owner

Remove commented-out code.

Remove commented-out code.
derppening marked this conversation as resolved
derppening requested changes 2024-06-19 18:29:12 +08:00
@ -2773,1 +2773,4 @@
}
/// Check if a [`Type`] is an ndarray.
fn is_type_ndarray(ctx: &mut CodeGenContext<'_, '_>, ty: Type) -> bool {
Collaborator

Use NDArrayType::is_instance to directly check on the LLVM value.

Use `NDArrayType::is_instance` to directly check on the LLVM value.
@ -2774,0 +2787,4 @@
/// Helper function to create `np.any()` and `np.all()`.
///
/// They are mixed together since they are extremely similar in terms of implementation.
fn helper_call_numpy_any_all<'ctx, G: CodeGenerator + ?Sized>(
Collaborator

This function belongs in codegen/numpy.rs.

This function belongs in `codegen/numpy.rs`.
Author
Collaborator

The function requires codegen::builtin_fns::unsupported_type, which is private. Should I move it too?

The function requires `codegen::builtin_fns::unsupported_type`, which is private. Should I move it too?
Collaborator

Follow how other numpy functions (e.g. np_copy) are implemented.

Follow how other numpy functions (e.g. `np_copy`) are implemented.
@ -2774,0 +2790,4 @@
fn helper_call_numpy_any_all<'ctx, G: CodeGenerator + ?Sized>(
generator: &mut G,
ctx: &mut CodeGenContext<'ctx, '_>,
kind: AnyOrAll,
Collaborator

Rather than having a kind, I would just make this function like so:

call_numpy_bool_reduce<'ctx, G, ReduceFn>(
    generator: &mut G
    ctx: &mut CodeGenContext<'ctx, '_>,
    (in_ty, in_val): (Type, BasicValueEnum<'ctx>),
    initial: IntValue<'ctx>,
    reduce_fn: ReduceFn,
    shortcircuit_if: Option<bool>,  // Can be Option<IntValue<'ctx>>
) -> Result<BasicValueEnum<'ctx>, String>
where
    G: CodeGenerator + ?Sized,
    ReduceFn: FnOnce(&mut G, &mut CodeGenContext<'ctx, '_>, (IntValue<'ctx>, IntValue<'ctx>)) -> IntValue<'ctx>,

So that it can be expressed like a reduction operation in functional programming (with the option to explicitly enable boolean short-circuiting).

Then this can be expressed as:

  • np_any: call_numpy_bool_reduce(generator, ctx, (in_ty, in_val), llvm_i1.const_zero(), /* |(acc, val)| { acc |= val }, Some(true)
  • np_all: call_numpy_bool_reduce(generator, ctx, (in_ty, in_val), llvm_i1.const_ones(), /* |(acc, val)| { acc &= val }, Some(false)
Rather than having a kind, I would just make this function like so: ```rs call_numpy_bool_reduce<'ctx, G, ReduceFn>( generator: &mut G ctx: &mut CodeGenContext<'ctx, '_>, (in_ty, in_val): (Type, BasicValueEnum<'ctx>), initial: IntValue<'ctx>, reduce_fn: ReduceFn, shortcircuit_if: Option<bool>, // Can be Option<IntValue<'ctx>> ) -> Result<BasicValueEnum<'ctx>, String> where G: CodeGenerator + ?Sized, ReduceFn: FnOnce(&mut G, &mut CodeGenContext<'ctx, '_>, (IntValue<'ctx>, IntValue<'ctx>)) -> IntValue<'ctx>, ``` So that it can be expressed like a reduction operation in functional programming (with the option to explicitly enable boolean short-circuiting). Then this can be expressed as: - `np_any`: `call_numpy_bool_reduce(generator, ctx, (in_ty, in_val), llvm_i1.const_zero(), /* |(acc, val)| { acc |= val }, Some(true)` - `np_all`: `call_numpy_bool_reduce(generator, ctx, (in_ty, in_val), llvm_i1.const_ones(), /* |(acc, val)| { acc &= val }, Some(false)`
Author
Collaborator

I would also like to take time to extend this to a general fold - to also refactor np.max() and np.min().

I would also like to take time to extend this to a general fold - to also refactor `np.max()` and `np.min()`.
Author
Collaborator

After some thoughts, I will not use folds to implement any/all. The short-circuiting logic was slightly awkward to implement. But more importantly it could not provide the necessary control to optimally implement np.any() & np.all() - they only have to set result to on_hit and then quit, rather than having to accumulate something to result for each scalar element within the ndarray.

After some thoughts, I will not use folds to implement any/all. The short-circuiting logic was slightly awkward to implement. But more importantly it could not provide the necessary control to optimally implement `np.any()` & `np.all()` - they only have to set `result` to `on_hit` and then quit, rather than having to accumulate something to `result` for each scalar element within the ndarray.
Collaborator

Address the rest of the comments are leave this to me then.

Address the rest of the comments are leave this to me then.
@ -2774,0 +2811,4 @@
/*
NOTE: `np.any(<empty ndarray>)` returns true.
NOTE: `np.all(<empty ndarray>)` returns false.
Collaborator

These two seem to be flipped.

>>> np.any(np.array([]))
False
>>> np.all(np.array([]))
True
These two seem to be flipped. ```py >>> np.any(np.array([])) False >>> np.all(np.array([])) True ```
derppening marked this conversation as resolved
@ -2774,0 +2835,4 @@
let llvm_i8 = ctx.ctx.i8_type();
let llvm_usize = generator.get_size_type(ctx.ctx);
let current_bb = ctx.builder.get_insert_block().unwrap();
Collaborator

Instead of doing this manually, I'd suggest extending gen_for_callback (and its family of functions) to also provide cont_bb and update_bb so that break and continue can be easily generated.

Instead of doing this manually, I'd suggest extending `gen_for_callback` (and its family of functions) to also provide `cont_bb` and `update_bb` so that `break` and `continue` can be easily generated.
Author
Collaborator

An isolated commit has now been created to extend gen_for_callback.

An isolated commit has now been created to extend `gen_for_callback`.
derppening marked this conversation as resolved
lyken force-pushed numpy-anyall from 554c9234b8 to ac5acf7156 2024-06-20 16:09:28 +08:00 Compare
lyken force-pushed numpy-anyall from ac5acf7156 to c4971fad82 2024-06-20 16:11:23 +08:00 Compare
lyken requested review from derppening 2024-06-20 16:23:41 +08:00
derppening requested changes 2024-06-21 13:38:01 +08:00
derppening left a comment
Collaborator

Please address all the comments and then assign this to me.

Please address all the comments and then assign this to me.
@ -1987,0 +1990,4 @@
///
/// * `ndarray`: The input ndarray [`NDArrayValue`]
/// * `body_fn`: A lambda containing IR statements that acts on every scalar element within `ndarray`.
pub fn gen_ndarray_iter_scalar_callback<'ctx, G, BodyFn>(
Collaborator

Please place this function somewhere before the gen_* family of functions.

Please place this function somewhere before the `gen_*` family of functions.
@ -462,6 +462,14 @@ pub fn gen_for<G: CodeGenerator>(
Ok(())
}
#[derive(Debug, Clone, Copy)]
Collaborator

Might as well derive Eq, PartialEq, Hash as well.

Might as well derive `Eq, PartialEq, Hash` as well.
Author
Collaborator

Understood. Will do soon, I am working on and am about to finish #432.

Understood. Will do soon, I am working on and am about to finish https://git.m-labs.hk/M-Labs/nac3/issues/432.
lyken force-pushed numpy-anyall from c4971fad82 to cf24bd5e0e 2024-06-21 15:12:38 +08:00 Compare
Author
Collaborator

Revised. Squashed to cleanup the branch history. Reassigning to @derppening.

Revised. Squashed to cleanup the branch history. Reassigning to @derppening.
derppening was assigned by lyken 2024-06-21 15:13:53 +08:00
derppening force-pushed numpy-anyall from cf24bd5e0e to b1e97aa2b0 2024-06-25 15:31:01 +08:00 Compare
This pull request has changes conflicting with the target branch.
  • nac3core/src/codegen/builtin_fns.rs
  • nac3core/src/codegen/classes.rs
  • nac3core/src/codegen/irrt/mod.rs
  • nac3core/src/codegen/numpy.rs
  • nac3core/src/codegen/stmt.rs
  • nac3core/src/toplevel/builtins.rs
  • nac3core/src/toplevel/helper.rs
  • nac3standalone/demo/interpret_demo.py
  • nac3standalone/demo/src/ndarray.py

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin numpy-anyall:numpy-anyall
git checkout numpy-anyall
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#424
No description provided.