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

Added indicator streams and reference indicators #60

Merged
merged 3 commits into from Mar 31, 2019
Merged

Conversation

codeplea
Copy link
Member

Indicator streams #57
Reference indicators #59

indicators.tcl now takes an extra parametr for each indicator. This option can be "ref" or "stream" or both. It'll generate code accordingly.

I used ATR as the first example. ATR is easy to make a reference for (call ti_tr, call ti_wilders) and it's easy (but not trivial) to make a streaming indicator for.

I updated smoke to check ref and stream if available.

I updated the Makefile to c99. It's a lot easier.

@codeplea
Copy link
Member Author

I'm open to suggestions on naming.

Also, I'm thinking that ti_xxx_stream_new() could return an error code if the options are bad (currently it just returns a null pointer).

Should ti_stream_run() ever return an error? Once the indicator is set up, what could go wrong?

smoke.c Outdated Show resolved Hide resolved
@rdbuf
Copy link

rdbuf commented Mar 31, 2019

It all looks good to me.

Yes, returning an error code is a good idea, we must be consistent.

In the code I've seen so far, all the testing for erroneous conditions was performed prior to any calculations, conceptually at the _new stage. Provided that we trust the input data (high >= low, e.g.), I can't think of any example of an error occurring during the computation. In either case, I don't think that we would like to test for anything inside of a streaming function, because it could potentially cause a noticeable performance hit.

@rdbuf
Copy link

rdbuf commented Mar 31, 2019

Have you benchmarked it yet?

Makefile Show resolved Hide resolved
@@ -272,15 +276,26 @@ long int ti_build();
typedef int (*ti_indicator_start_function)($fun_args_start);
typedef int (*ti_indicator_function)($fun_args);


struct ti_stream; typedef struct ti_stream ti_stream;
typedef int (*ti_indicator_stream_new)($fun_stream_new_args);
Copy link

Choose a reason for hiding this comment

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

why not to return the pointer just like malloc does? we would still be able to check for errors, wouldn't we?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can only have one return value, so it's either:

int stream_new(TI_REAL const *options, ti_stream **stream);

or

ti_stream *stream_new(TI_REAL const *options, int *return_code);

I don't think either is better or worse, really. The normal functions return int, so I just did that for consistency.

Copy link

Choose a reason for hiding this comment

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

I think returning "ti_stream" would have been a better choice, since that's the actual return type and that is what the client needs to pass again and again to compute streaming indicator. Its req. in get_info/ progress/ stream_run / stream_free.

@codeplea
Copy link
Member Author

Have you benchmarked it yet?

I haven't benchmarked anything. I think the next step is to integrate this with the benchmark program.

Also, either expand benchmark or write a new program which runs random data and options and compares the old indicators to the reference indicators to the stream indicators. I'll leave that up to you, if you want to work on it.

Finally, it should be integrated into fuzzer too. It's going to be really easy to make memory errors with the streaming indicators. Running fuzzer with valgrind will be a great way to find those.

I can't think of any example of an error occurring during the computation.

I think it would be very uncommon, but we may as well keep the return value. Maybe it'll come in handy at some point. About the only thing I can imagine is if a really complicated indicator needs to do memory allocation and that fails.

The other failure cases should usually be handled by just returning NaNs.

@codeplea
Copy link
Member Author

I changed smoke so that it test streaming indicators both 1 bar at a time and all the bars at once. The streaming code is complicated enough that I think it's import to make sure it goes something both ways.

@codeplea codeplea merged commit bdc09f9 into 0.9 Mar 31, 2019
@codeplea codeplea deleted the stream_ref branch March 31, 2019 04:51
@codeplea
Copy link
Member Author

Merged to 0.9.

I won't have time to work on TI for a while. Feel free to build on it.

@rdbuf
Copy link

rdbuf commented Mar 31, 2019

I will. Thanks for the work done!

@rdbuf rdbuf mentioned this pull request Apr 4, 2019
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

Successfully merging this pull request may close these issues.

None yet

3 participants