computablee / DotMP

A collection of powerful abstractions for parallel programming in .NET with an OpenMP-like API.
https://computablee.github.io/DotMP/
GNU Lesser General Public License v2.1
29 stars 7 forks source link

Need more rigorous error checking #51

Closed computablee closed 1 year ago

computablee commented 1 year ago

It would be great if the project employed some more rigorous error checking.

Some situations that come to mind that should throw exceptions:

  1. end < start in Parallel.For, Parallel.ForReduction, and Parallel.Taskloop
  2. start < 0 || end < 0 in Parallel.For, Parallel.ForReduction, and Parallel.Taskloop
  3. Parallel.Ordered used outside Parallel.For or Parallel.ForReduction
  4. num_threads == 0 in Parallel.ParallelRegion
  5. chunk_size == 0 in Parallel.For or Parallel.ForReduction
  6. num_tasks == 0 || grainsize == 0 in Parallel.Taskloop

It would be nice to go through and start adding some more exceptions and throw them. We should try to avoid internal uncaught library exceptions when possible, and pass informed errors onto the user.

Kaybangz commented 1 year ago

Hello, I would like to try my hands on this issue.

If there's anything else you think I need to know about the issue, I'd appreciate you telling me.

computablee commented 1 year ago

@Kaybangz Nothing immediately comes to mind, just follow good practices and try to keep code modular. It may be worth creating a general function to check all this stuff instead of individually checking the values in each method.

I'll assign you.

computablee commented 1 year ago

@Kaybangz Just checking in, do you need help with anything? Do you have any updates?

Kaybangz commented 1 year ago

@computablee I apologize for the delay, but I'm currently unable to work on the issue due to an emergency. Forgive me

computablee commented 1 year ago

No worries! And no need to ask for forgiveness. Do you want me to reassign the issue to someone else?

Kaybangz commented 1 year ago

I can start working on the issue today

computablee commented 1 year ago

Sure thing, and no rush! I didn't mean to come off as pushy, I just wanted to check in. Take your time, and let me know if you need anything from me :)

Kaybangz commented 1 year ago

It would be great if the project employed some more rigorous error checking.

Some situations that come to mind that should throw exceptions:

  1. end < start in Parallel.For, Parallel.ForReduction, and Parallel.Taskloop
  2. start < 0 || end < 0 in Parallel.For, Parallel.ForReduction, and Parallel.Taskloop
  3. Parallel.Ordered used outside Parallel.For or Parallel.ForReduction
  4. num_threads == 0 in Parallel.ParallelRegion
  5. chunk_size == 0 in Parallel.For or Parallel.ForReduction
  6. num_tasks == 0 || grainsize == 0 in Parallel.Taskloop

It would be nice to go through and start adding some more exceptions and throw them. We should try to avoid internal uncaught library exceptions when possible, and pass informed errors onto the user.

I have handled exceptions for all the others except this: 3. Parallel.Ordered used outside Parallel.For or Parallel.ForReduction

Can you please provide more clarity

computablee commented 1 year ago

@Kaybangz Sure.

Parallel.Ordered is a construct that should only run inside of a Parallel.For region, a Parallel.ForReduction region, or a region that wraps one of those. In essence, this means that the following DotMP is valid:

public void DoWork()
{
    DotMP.Parallel.ParallelFor(0, N, i => //ParallelFor is a wrapper around ParallelRegion and For
    {
        DotMP.Parallel.Ordered(0, () =>
        {
            // code here
        });
    });
}

But the following is not valid:

// assuming DoWork is not called from a For or ForReduction
public void DoWork()
{
    DotMP.Parallel.Ordered(0, () =>
    {
        // code here
    });
}

To check if a method is being called from a For or ForReduction, you should check out some of the code that was implemented for Parallel.Single to ensure that singles can only be used outside of a For/ForReduction: #47. Basically, the WorkShare singleton exposes a property called in_for that can be read to determine if the current executing code is within a For or ForReduction. You should be able to just read this property and provide a simple if/then error check.

Kaybangz commented 1 year ago

Thank you very much, this helps. One more question, which exception should be thrown? I used a basic ArgumentException for 1,2,4,5,6

computablee commented 1 year ago

Good question, let's add a new exception to Exceptions.cs called NotInForException. Just follow the format of Exceptions.cs and add a simple class-level comment. You can reuse the comments for the constructors.