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

tests for smart_read #57

Closed
MaceKuailv opened this issue Jun 19, 2023 · 6 comments
Closed

tests for smart_read #57

MaceKuailv opened this issue Jun 19, 2023 · 6 comments
Labels

Comments

@MaceKuailv
Copy link
Owner

New smart_read break notebooks. Due to the lack of comprehensive tests, the new version of smart_read (especially with slicing) read-out wrong values, and in some cases it raises error. It might be easier if I have some qualitative tests first and then look at this again.

For now, I fall back to the previous, slower version to finish up this part of development.

Yeah, it is extremely frustrating.

@ThomasHaine
Copy link
Collaborator

Is this related to the LLC4320 demo notebook error?

@MaceKuailv
Copy link
Owner Author

The short answer is: Maybe, but not likely.

@ThomasHaine There are two parts to the simulation. 1. read; 2. execute. The way the execute part is designed is that whatever results the reading part gives it should not have that error.

That said, I did see similar errors yesterday when I was tweaking smart_read and some other parts of the code. So far, I have made the execution part of the code more logical, and hopefully faster for simulation like the one we run with LLC4320. It is possible that this version of the code already does not have the same error as before. Even if that is the case, we do not know whether it is smart_read or some other parts. The LLC4320 error is extremely rare, and I haven't figured out a way to reproduce it easily.

@malmans2
Copy link
Contributor

mmm, if I didn't introduce a bug, these are the two things I can think about:

  1. Somehow the indexes passed to smart_read are outside the domain of da?
  2. My version of smart_read modifies something in-place?

The only additional check I've done while rewriting smart read was to make sure to get identical results with the simple case I used for benchmarking (details should be in the PR).

@MaceKuailv
Copy link
Owner Author

The slicing part does not work when there is negative indices. It also produces error in some other cases (not within the function).
The main reason I pin it back in the PR is that there is just a lot of hustle and bustle going on and things are breaking. I just wanted to single things out.

At the end of the day, I would want the smart_read to be as modular as possible. This issue is just a reminder to myself to fix everything around the function and test it properly.

@malmans2
Copy link
Contributor

Ah ok, makes sense. I assumed smart_read was receiving positive indexes.

@MaceKuailv MaceKuailv mentioned this issue Jun 30, 2023
@MaceKuailv
Copy link
Owner Author

resolved with PR #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants